diff mbox series

[08/21] Hexagon (target/hexagon) Clean up pred_written usage

Message ID 20230426004202.1319250-1-tsimpson@quicinc.com
State New
Headers show
Series Hexagon (target/hexagon) short-circuit and move to DisasContext | expand

Commit Message

Taylor Simpson April 26, 2023, 12:42 a.m. UTC 
Only endloop instructions will conditionally write to a predicate.
When there is an endloop instruction, we preload the values into
new_pred_value.

The only place pred_written is needed is when HEX_DEBUG is on.

We remove the last use of check_for_attrib.  However, new uses will be
introduced later in this series, so we change it to "static inline".

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/genptr.c    | 16 +++++-------
 target/hexagon/translate.c | 53 ++++++++++++--------------------------
 2 files changed, 23 insertions(+), 46 deletions(-)

Comments

Richard Henderson April 26, 2023, 9:23 p.m. UTC | #1 
On 4/26/23 01:42, Taylor Simpson wrote:
> Only endloop instructions will conditionally write to a predicate.
> When there is an endloop instruction, we preload the values into
> new_pred_value.
> 
> The only place pred_written is needed is when HEX_DEBUG is on.
> 
> We remove the last use of check_for_attrib.  However, new uses will be
> introduced later in this series, so we change it to "static inline".

This is insufficient -- clang will warn about unused inline functions within the main C 
file (as opposed to #included).

Use __attribute__((unused)) instead, and remove it when it gains new unconditional uses.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Taylor Simpson April 26, 2023, 9:27 p.m. UTC | #2 
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, April 26, 2023 4:23 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@linaro.org; ale@rev.ng; anjo@rev.ng; Brian Cain
> <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>
> Subject: Re: [PATCH 08/21] Hexagon (target/hexagon) Clean up
> pred_written usage
> 
> On 4/26/23 01:42, Taylor Simpson wrote:
> > Only endloop instructions will conditionally write to a predicate.
> > When there is an endloop instruction, we preload the values into
> > new_pred_value.
> >
> > The only place pred_written is needed is when HEX_DEBUG is on.
> >
> > We remove the last use of check_for_attrib.  However, new uses will be
> > introduced later in this series, so we change it to "static inline".
> 
> This is insufficient -- clang will warn about unused inline functions within the
> main C file (as opposed to #included).
> 
> Use __attribute__((unused)) instead, and remove it when it gains new
> unconditional uses.

Is it OK to use the attribute itself or is G_GNUC_UNUSED preferred?


> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks,
Taylor
Richard Henderson April 27, 2023, 7:37 a.m. UTC | #3 
On 4/26/23 22:27, Taylor Simpson wrote:
>>> We remove the last use of check_for_attrib.  However, new uses will be
>>> introduced later in this series, so we change it to "static inline".
>>
>> This is insufficient -- clang will warn about unused inline functions within the
>> main C file (as opposed to #included).
>>
>> Use __attribute__((unused)) instead, and remove it when it gains new
>> unconditional uses.
> 
> Is it OK to use the attribute itself or is G_GNUC_UNUSED preferred?

I've been using the attribute itself in my patches, e.g.

https://lore.kernel.org/qemu-devel/20230424054105.1579315-1-richard.henderson@linaro.org/T/#mb07f0d1393f7f0a252f459dca6a91a2162581281

But either is fine with me.


r~
diff mbox series

Patch

diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 104ff061f5..b9c4a46e3a 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -137,7 +137,9 @@  void gen_log_pred_write(DisasContext *ctx, int pnum, TCGv val)
         tcg_gen_and_tl(hex_new_pred_value[pnum],
                        hex_new_pred_value[pnum], base_val);
     }
-    tcg_gen_ori_tl(hex_pred_written, hex_pred_written, 1 << pnum);
+    if (HEX_DEBUG) {
+        tcg_gen_ori_tl(hex_pred_written, hex_pred_written, 1 << pnum);
+    }
     set_bit(pnum, ctx->pregs_written);
 }
 
@@ -823,15 +825,13 @@  static void gen_endloop0(DisasContext *ctx)
 
     /*
      *    if (lpcfg == 1) {
-     *        hex_new_pred_value[3] = 0xff;
-     *        hex_pred_written |= 1 << 3;
+     *        p3 = 0xff;
      *    }
      */
     TCGLabel *label1 = gen_new_label();
     tcg_gen_brcondi_tl(TCG_COND_NE, lpcfg, 1, label1);
     {
-        tcg_gen_movi_tl(hex_new_pred_value[3], 0xff);
-        tcg_gen_ori_tl(hex_pred_written, hex_pred_written, 1 << 3);
+        gen_log_pred_write(ctx, 3, tcg_constant_tl(0xff));
     }
     gen_set_label(label1);
 
@@ -900,14 +900,12 @@  static void gen_endloop01(DisasContext *ctx)
 
     /*
      *    if (lpcfg == 1) {
-     *        hex_new_pred_value[3] = 0xff;
-     *        hex_pred_written |= 1 << 3;
+     *        p3 = 0xff;
      *    }
      */
     tcg_gen_brcondi_tl(TCG_COND_NE, lpcfg, 1, label1);
     {
-        tcg_gen_movi_tl(hex_new_pred_value[3], 0xff);
-        tcg_gen_ori_tl(hex_pred_written, hex_pred_written, 1 << 3);
+        gen_log_pred_write(ctx, 3, tcg_constant_tl(0xff));
     }
     gen_set_label(label1);
 
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index c087f183d0..1f04559f91 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -239,7 +239,7 @@  static int read_packet_words(CPUHexagonState *env, DisasContext *ctx,
     return nwords;
 }
 
-static bool check_for_attrib(Packet *pkt, int attrib)
+static inline bool check_for_attrib(Packet *pkt, int attrib)
 {
     for (int i = 0; i < pkt->num_insns; i++) {
         if (GET_ATTRIB(pkt->insn[i].opcode, attrib)) {
@@ -262,11 +262,6 @@  static bool need_slot_cancelled(Packet *pkt)
     return false;
 }
 
-static bool need_pred_written(Packet *pkt)
-{
-    return check_for_attrib(pkt, A_WRITES_PRED_REG);
-}
-
 static bool need_next_PC(DisasContext *ctx)
 {
     Packet *pkt = ctx->pkt;
@@ -414,7 +409,7 @@  static void gen_start_packet(DisasContext *ctx)
             tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], next_PC);
         }
     }
-    if (need_pred_written(pkt)) {
+    if (HEX_DEBUG) {
         tcg_gen_movi_tl(hex_pred_written, 0);
     }
 
@@ -428,6 +423,17 @@  static void gen_start_packet(DisasContext *ctx)
         }
     }
 
+    /*
+     * Preload the predicated pred registers into hex_new_pred_value[pred_num]
+     * Only endloop instructions conditionally write to pred registers
+     */
+    if (pkt->pkt_has_endloop) {
+        for (int i = 0; i < ctx->preg_log_idx; i++) {
+            int pred_num = ctx->preg_log[i];
+            tcg_gen_mov_tl(hex_new_pred_value[pred_num], hex_pred[pred_num]);
+        }
+    }
+
     /* Preload the predicated HVX registers into future_VRegs and tmp_VRegs */
     if (!bitmap_empty(ctx->predicated_future_vregs, NUM_VREGS)) {
         int i = find_first_bit(ctx->predicated_future_vregs, NUM_VREGS);
@@ -532,41 +538,14 @@  static void gen_reg_writes(DisasContext *ctx)
 
 static void gen_pred_writes(DisasContext *ctx)
 {
-    int i;
-
     /* Early exit if the log is empty */
     if (!ctx->preg_log_idx) {
         return;
     }
 
-    /*
-     * Only endloop instructions will conditionally
-     * write a predicate.  If there are no endloop
-     * instructions, we can use the non-conditional
-     * write of the predicates.
-     */
-    if (ctx->pkt->pkt_has_endloop) {
-        TCGv zero = tcg_constant_tl(0);
-        TCGv pred_written = tcg_temp_new();
-        for (i = 0; i < ctx->preg_log_idx; i++) {
-            int pred_num = ctx->preg_log[i];
-
-            tcg_gen_andi_tl(pred_written, hex_pred_written, 1 << pred_num);
-            tcg_gen_movcond_tl(TCG_COND_NE, hex_pred[pred_num],
-                               pred_written, zero,
-                               hex_new_pred_value[pred_num],
-                               hex_pred[pred_num]);
-        }
-    } else {
-        for (i = 0; i < ctx->preg_log_idx; i++) {
-            int pred_num = ctx->preg_log[i];
-            tcg_gen_mov_tl(hex_pred[pred_num], hex_new_pred_value[pred_num]);
-            if (HEX_DEBUG) {
-                /* Do this so HELPER(debug_commit_end) will know */
-                tcg_gen_ori_tl(hex_pred_written, hex_pred_written,
-                               1 << pred_num);
-            }
-        }
+    for (int i = 0; i < ctx->preg_log_idx; i++) {
+        int pred_num = ctx->preg_log[i];
+        tcg_gen_mov_tl(hex_pred[pred_num], hex_new_pred_value[pred_num]);
     }
 }