[PATCH 0/2] Fix usage of GETPC(), variable shadowing

[PATCH 0/2] Fix usage of GETPC(), variable shadowing

[Brian Cain]

Matheus' patch has previously been reviewed, but I based my -Wshadow
patch on his.  So I'm submitting the series for review.

Brian Cain (1):
  target/hexagon: fix some occurrences of -Wshadow=local

Matheus Tavares Bernardino (1):
  target/hexagon: move GETPC() calls to top level helpers

 target/hexagon/imported/alu.idef |  6 +--
 target/hexagon/macros.h          | 19 ++++----
 target/hexagon/mmvec/macros.h    |  2 +-
 target/hexagon/op_helper.c       | 84 ++++++++++++--------------------
 target/hexagon/op_helper.h       |  9 ----
 target/hexagon/translate.c       | 10 ++--
 6 files changed, 50 insertions(+), 80 deletions(-)

-- 
2.25.1

[PATCH 1/2] target/hexagon: move GETPC() calls to top level helpers

[Brian Cain]

From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>

As docs/devel/loads-stores.rst states:

  ``GETPC()`` should be used with great care: calling
  it in other functions that are *not* the top level
  ``HELPER(foo)`` will cause unexpected behavior. Instead, the
  value of ``GETPC()`` should be read from the helper and passed
  if needed to the functions that the helper calls.

Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are a few snippets where that is not currently the case:

- probe_store(), which is only called from two helpers, so it's easy to
  move GETPC() up.

- mem_load*() functions, which are also called directly from helpers,
  but through the MEM_LOAD*() set of macros. Note that this are only
  used when compiling with --disable-hexagon-idef-parser.

  In this case, we also take this opportunity to simplify the code,
  unifying the mem_load*() functions.

- HELPER(probe_hvx_stores), when called from another helper, ends up
  using its own GETPC() expansion instead of the top level caller.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
Message-Id: <2c74c3696946edba7cc5b2942cf296a5af532052.1689070412.git.quic_mathbern@quicinc.com>-ne
Reviewed-by: Brian Cain <bcain@quicinc.com>
---
 target/hexagon/macros.h    | 19 +++++-----
 target/hexagon/op_helper.c | 75 +++++++++++++++-----------------------
 target/hexagon/op_helper.h |  9 -----
 3 files changed, 38 insertions(+), 65 deletions(-)

diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..dafa0df6ed 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
 #define MEM_STORE8(VA, DATA, SLOT) \
     MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
 #else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
 #define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
 #define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
 #define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val, int shift)
 #ifdef QEMU_GENERATE
 #define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
 #else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
 #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-    DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+    do { \
+        check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+        DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+    } while (0)
 #endif
 
 #define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..8ca3976a65 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env, int slot, int check)
     }
 }
 
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static void commit_store(CPUHexagonState *env, int slot_num, uintptr_t ra)
 {
-    uintptr_t ra = GETPC();
     uint8_t width = env->mem_log_stores[slot_num].width;
     target_ulong va = env->mem_log_stores[slot_num].va;
 
@@ -119,6 +118,12 @@ void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
     }
 }
 
+void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+{
+    uintptr_t ra = GETPC();
+    commit_store(env, slot_num, ra);
+}
+
 void HELPER(gather_store)(CPUHexagonState *env, uint32_t addr, int slot)
 {
     mem_gather_store(env, addr, slot);
@@ -467,13 +472,12 @@ int32_t HELPER(cabacdecbin_pred)(int64_t RssV, int64_t RttV)
 }
 
 static void probe_store(CPUHexagonState *env, int slot, int mmu_idx,
-                        bool is_predicated)
+                        bool is_predicated, uintptr_t retaddr)
 {
     if (!is_predicated || !(env->slot_cancelled & (1 << slot))) {
         size1u_t width = env->mem_log_stores[slot].width;
         target_ulong va = env->mem_log_stores[slot].va;
-        uintptr_t ra = GETPC();
-        probe_write(env, va, width, mmu_idx, ra);
+        probe_write(env, va, width, mmu_idx, retaddr);
     }
 }
 
@@ -494,12 +498,13 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int args)
     int mmu_idx = FIELD_EX32(args, PROBE_PKT_SCALAR_STORE_S0, MMU_IDX);
     bool is_predicated =
         FIELD_EX32(args, PROBE_PKT_SCALAR_STORE_S0, IS_PREDICATED);
-    probe_store(env, 0, mmu_idx, is_predicated);
+    uintptr_t ra = GETPC();
+    probe_store(env, 0, mmu_idx, is_predicated, ra);
 }
 
-void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
+static void probe_hvx_stores(CPUHexagonState *env, int mmu_idx,
+                                    uintptr_t retaddr)
 {
-    uintptr_t retaddr = GETPC();
     int i;
 
     /* Normal (possibly masked) vector store */
@@ -538,6 +543,12 @@ void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
     }
 }
 
+void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
+{
+    uintptr_t retaddr = GETPC();
+    probe_hvx_stores(env, mmu_idx, retaddr);
+}
+
 void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
 {
     bool has_st0 = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, HAS_ST0);
@@ -547,18 +558,20 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
     bool s0_is_pred = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, S0_IS_PRED);
     bool s1_is_pred = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, S1_IS_PRED);
     int mmu_idx = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, MMU_IDX);
+    uintptr_t ra = GETPC();
 
     if (has_st0) {
-        probe_store(env, 0, mmu_idx, s0_is_pred);
+        probe_store(env, 0, mmu_idx, s0_is_pred, ra);
     }
     if (has_st1) {
-        probe_store(env, 1, mmu_idx, s1_is_pred);
+        probe_store(env, 1, mmu_idx, s1_is_pred, ra);
     }
     if (has_hvx_stores) {
-        HELPER(probe_hvx_stores)(env, mmu_idx);
+        probe_hvx_stores(env, mmu_idx, ra);
     }
 }
 
+#ifndef CONFIG_HEXAGON_IDEF_PARSER
 /*
  * mem_noshuf
  * Section 5.5 of the Hexagon V67 Programmer's Reference Manual
@@ -567,46 +580,16 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
  * wasn't cancelled), we have to do the store first.
  */
 static void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
-                         uint32_t slot, target_ulong vaddr, int size)
+                         uint32_t slot, target_ulong vaddr, int size,
+                         uintptr_t ra)
 {
     if (slot == 0 && pkt_has_store_s1 &&
         ((env->slot_cancelled & (1 << 1)) == 0)) {
-        HELPER(probe_noshuf_load)(env, vaddr, size, MMU_USER_IDX);
-        HELPER(commit_store)(env, 1);
+        probe_read(env, vaddr, size, MMU_USER_IDX, ra);
+        commit_store(env, 1, ra);
     }
 }
-
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
-                  uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 1);
-    return cpu_ldub_data_ra(env, vaddr, ra);
-}
-
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 2);
-    return cpu_lduw_data_ra(env, vaddr, ra);
-}
-
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 4);
-    return cpu_ldl_data_ra(env, vaddr, ra);
-}
-
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 8);
-    return cpu_ldq_data_ra(env, vaddr, ra);
-}
+#endif
 
 /* Floating point */
 float64 HELPER(conv_sf2df)(CPUHexagonState *env, float32 RsV)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..66119cf3d4 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,6 @@
 #define HEXAGON_OP_HELPER_H
 
 /* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
-                  uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr);
-
 void log_store64(CPUHexagonState *env, target_ulong addr,
                  int64_t val, int width, int slot);
 void log_store32(CPUHexagonState *env, target_ulong addr,
-- 
2.25.1

[PATCH 2/2] target/hexagon: fix some occurrences of -Wshadow=local

[Brian Cain]

Of the changes in this commit, the changes in `HELPER(commit_hvx_stores)()`
are less obvious.  They are required because of some macro invocations like
SCATTER_OP_WRITE_TO_MEM().

e.g.:

    In file included from ../target/hexagon/op_helper.c:31:
    ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’ shadows a previous local [-Werror=shadow=compatible-local]
      205 |         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
          |                  ^
    ../target/hexagon/op_helper.c:157:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
      157 |                 SCATTER_OP_WRITE_TO_MEM(uint16_t);
          |                 ^~~~~~~~~~~~~~~~~~~~~~~
    ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
      135 |     int i;
          |         ^
    In file included from ../target/hexagon/op_helper.c:31:
    ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’ shadows a previous local [-Werror=shadow=compatible-local]
      204 |         uintptr_t ra = GETPC(); \
          |                   ^~
    ../target/hexagon/op_helper.c:160:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
      160 |                 SCATTER_OP_WRITE_TO_MEM(uint32_t);
          |                 ^~~~~~~~~~~~~~~~~~~~~~~
    ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
      134 |     uintptr_t ra = GETPC();
          |               ^~

Reviewed-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Brian Cain <bcain@quicinc.com>
---
 target/hexagon/imported/alu.idef |  6 +++---
 target/hexagon/mmvec/macros.h    |  2 +-
 target/hexagon/op_helper.c       |  9 +++------
 target/hexagon/translate.c       | 10 +++++-----
 4 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/target/hexagon/imported/alu.idef b/target/hexagon/imported/alu.idef
index 12d2aac5d4..b855676989 100644
--- a/target/hexagon/imported/alu.idef
+++ b/target/hexagon/imported/alu.idef
@@ -1142,9 +1142,9 @@ Q6INSN(A4_cround_rr,"Rd32=cround(Rs32,Rt32)",ATTRIBS(),"Convergent Round", {RdV
             tmp128 = fSHIFTR128(tmp128, SHIFT);\
             DST =  fCAST16S_8S(tmp128);\
         } else {\
-            size16s_t rndbit_128 =  fCAST8S_16S((1LL << (SHIFT - 1))); \
-            size16s_t src_128 =  fCAST8S_16S(SRC); \
-            size16s_t tmp128 = fADD128(src_128, rndbit_128);\
+            rndbit_128 =  fCAST8S_16S((1LL << (SHIFT - 1))); \
+            src_128 =  fCAST8S_16S(SRC); \
+            tmp128 = fADD128(src_128, rndbit_128);\
             tmp128 = fSHIFTR128(tmp128, SHIFT);\
             DST =  fCAST16S_8S(tmp128);\
         }
diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index a655634fd1..1ceb9453ee 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -201,7 +201,7 @@
     } while (0)
 #define SCATTER_OP_WRITE_TO_MEM(TYPE) \
     do { \
-        uintptr_t ra = GETPC(); \
+        ra = GETPC(); \
         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
             if (test_bit(i, env->vtcm_log.mask)) { \
                 TYPE dst = 0; \
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 8ca3976a65..da10ac5847 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -132,10 +132,9 @@ void HELPER(gather_store)(CPUHexagonState *env, uint32_t addr, int slot)
 void HELPER(commit_hvx_stores)(CPUHexagonState *env)
 {
     uintptr_t ra = GETPC();
-    int i;
 
     /* Normal (possibly masked) vector store */
-    for (i = 0; i < VSTORES_MAX; i++) {
+    for (int i = 0; i < VSTORES_MAX; i++) {
         if (env->vstore_pending[i]) {
             env->vstore_pending[i] = 0;
             target_ulong va = env->vstore[i].va;
@@ -162,7 +161,7 @@ void HELPER(commit_hvx_stores)(CPUHexagonState *env)
                 g_assert_not_reached();
             }
         } else {
-            for (i = 0; i < sizeof(MMVector); i++) {
+            for (int i = 0; i < sizeof(MMVector); i++) {
                 if (test_bit(i, env->vtcm_log.mask)) {
                     cpu_stb_data_ra(env, env->vtcm_log.va[i],
                                     env->vtcm_log.data.ub[i], ra);
@@ -505,10 +504,8 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int args)
 static void probe_hvx_stores(CPUHexagonState *env, int mmu_idx,
                                     uintptr_t retaddr)
 {
-    int i;
-
     /* Normal (possibly masked) vector store */
-    for (i = 0; i < VSTORES_MAX; i++) {
+    for (int i = 0; i < VSTORES_MAX; i++) {
         if (env->vstore_pending[i]) {
             target_ulong va = env->vstore[i].va;
             int size = env->vstore[i].size;
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index c00254e4d5..a1c7cd6f21 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -553,7 +553,7 @@ static void gen_start_packet(DisasContext *ctx)
     /* Preload the predicated registers into get_result_gpr(ctx, i) */
     if (ctx->need_commit &&
         !bitmap_empty(ctx->predicated_regs, TOTAL_PER_THREAD_REGS)) {
-        int i = find_first_bit(ctx->predicated_regs, TOTAL_PER_THREAD_REGS);
+        i = find_first_bit(ctx->predicated_regs, TOTAL_PER_THREAD_REGS);
         while (i < TOTAL_PER_THREAD_REGS) {
             tcg_gen_mov_tl(get_result_gpr(ctx, i), hex_gpr[i]);
             i = find_next_bit(ctx->predicated_regs, TOTAL_PER_THREAD_REGS,
@@ -566,7 +566,7 @@ static void gen_start_packet(DisasContext *ctx)
      * Only endloop instructions conditionally write to pred registers
      */
     if (ctx->need_commit && pkt->pkt_has_endloop) {
-        for (int i = 0; i < ctx->preg_log_idx; i++) {
+        for (i = 0; i < ctx->preg_log_idx; i++) {
             int pred_num = ctx->preg_log[i];
             ctx->new_pred_value[pred_num] = tcg_temp_new();
             tcg_gen_mov_tl(ctx->new_pred_value[pred_num], hex_pred[pred_num]);
@@ -575,7 +575,7 @@ static void gen_start_packet(DisasContext *ctx)
 
     /* 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);
+        i = find_first_bit(ctx->predicated_future_vregs, NUM_VREGS);
         while (i < NUM_VREGS) {
             const intptr_t VdV_off =
                 ctx_future_vreg_off(ctx, i, 1, true);
@@ -588,7 +588,7 @@ static void gen_start_packet(DisasContext *ctx)
         }
     }
     if (!bitmap_empty(ctx->predicated_tmp_vregs, NUM_VREGS)) {
-        int i = find_first_bit(ctx->predicated_tmp_vregs, NUM_VREGS);
+        i = find_first_bit(ctx->predicated_tmp_vregs, NUM_VREGS);
         while (i < NUM_VREGS) {
             const intptr_t VdV_off =
                 ctx_tmp_vreg_off(ctx, i, 1, true);
@@ -1228,7 +1228,7 @@ void hexagon_translate_init(void)
             offsetof(CPUHexagonState, mem_log_stores[i].data64),
             store_val64_names[i]);
     }
-    for (int i = 0; i < VSTORES_MAX; i++) {
+    for (i = 0; i < VSTORES_MAX; i++) {
         snprintf(vstore_addr_names[i], NAME_LEN, "vstore_addr_%d", i);
         hex_vstore_addr[i] = tcg_global_mem_new(cpu_env,
             offsetof(CPUHexagonState, vstore[i].va),
-- 
2.25.1

Re: [PATCH 0/2] Fix usage of GETPC(), variable shadowing

[Anton Johansson via]

On 04/10/23, Brian Cain wrote:
> Matheus' patch has previously been reviewed, but I based my -Wshadow
> patch on his.  So I'm submitting the series for review.
> 
> Brian Cain (1):
>   target/hexagon: fix some occurrences of -Wshadow=local
> 
> Matheus Tavares Bernardino (1):
>   target/hexagon: move GETPC() calls to top level helpers
> 
>  target/hexagon/imported/alu.idef |  6 +--
>  target/hexagon/macros.h          | 19 ++++----
>  target/hexagon/mmvec/macros.h    |  2 +-
>  target/hexagon/op_helper.c       | 84 ++++++++++++--------------------
>  target/hexagon/op_helper.h       |  9 ----
>  target/hexagon/translate.c       | 10 ++--
>  6 files changed, 50 insertions(+), 80 deletions(-)
> 
> -- 
> 2.25.1
> 

Both patches:
Reviewed-by: Anton Johansson <anjo@rev.ng>
Tested-by: Anton Johansson <anjo@rev.ng>

Re: [PATCH 0/2] Fix usage of GETPC(), variable shadowing

[Anton Johansson]

On 04/10/23, Brian Cain wrote:
> Matheus' patch has previously been reviewed, but I based my -Wshadow
> patch on his.  So I'm submitting the series for review.
> 
> Brian Cain (1):
>   target/hexagon: fix some occurrences of -Wshadow=local
> 
> Matheus Tavares Bernardino (1):
>   target/hexagon: move GETPC() calls to top level helpers
> 
>  target/hexagon/imported/alu.idef |  6 +--
>  target/hexagon/macros.h          | 19 ++++----
>  target/hexagon/mmvec/macros.h    |  2 +-
>  target/hexagon/op_helper.c       | 84 ++++++++++++--------------------
>  target/hexagon/op_helper.h       |  9 ----
>  target/hexagon/translate.c       | 10 ++--
>  6 files changed, 50 insertions(+), 80 deletions(-)
> 
> -- 
> 2.25.1
> 

Both patches:
Reviewed-by: Anton Johansson <anjo@rev.ng>
Tested-by: Anton Johansson <anjo@rev.ng>

Re: [PATCH 0/2] Fix usage of GETPC(), variable shadowing

[Markus Armbruster]

Brian Cain <bcain@quicinc.com> writes:

> Matheus' patch has previously been reviewed, but I based my -Wshadow
> patch on his.  So I'm submitting the series for review.

I'm not queueing these, because it's more than just shadowing.  I trust
your PR will arrive in due time :)

Thanks!

Re: [PATCH 2/2] target/hexagon: fix some occurrences of -Wshadow=local

[Philippe Mathieu-Daudé]

Hi Matheus,

On 4/10/23 14:39, Brian Cain wrote:
> Of the changes in this commit, the changes in `HELPER(commit_hvx_stores)()`
> are less obvious.  They are required because of some macro invocations like
> SCATTER_OP_WRITE_TO_MEM().
> 
> e.g.:
> 
>      In file included from ../target/hexagon/op_helper.c:31:
>      ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’ shadows a previous local [-Werror=shadow=compatible-local]
>        205 |         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
>            |                  ^
>      ../target/hexagon/op_helper.c:157:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
>        157 |                 SCATTER_OP_WRITE_TO_MEM(uint16_t);
>            |                 ^~~~~~~~~~~~~~~~~~~~~~~
>      ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
>        135 |     int i;
>            |         ^
>      In file included from ../target/hexagon/op_helper.c:31:
>      ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’ shadows a previous local [-Werror=shadow=compatible-local]
>        204 |         uintptr_t ra = GETPC(); \
>            |                   ^~
>      ../target/hexagon/op_helper.c:160:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
>        160 |                 SCATTER_OP_WRITE_TO_MEM(uint32_t);
>            |                 ^~~~~~~~~~~~~~~~~~~~~~~
>      ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
>        134 |     uintptr_t ra = GETPC();
>            |               ^~
> 
> Reviewed-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> ---
>   target/hexagon/imported/alu.idef |  6 +++---
>   target/hexagon/mmvec/macros.h    |  2 +-
>   target/hexagon/op_helper.c       |  9 +++------
>   target/hexagon/translate.c       | 10 +++++-----
>   4 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/target/hexagon/imported/alu.idef b/target/hexagon/imported/alu.idef
> index 12d2aac5d4..b855676989 100644
> --- a/target/hexagon/imported/alu.idef
> +++ b/target/hexagon/imported/alu.idef
> @@ -1142,9 +1142,9 @@ Q6INSN(A4_cround_rr,"Rd32=cround(Rs32,Rt32)",ATTRIBS(),"Convergent Round", {RdV
>               tmp128 = fSHIFTR128(tmp128, SHIFT);\
>               DST =  fCAST16S_8S(tmp128);\
>           } else {\
> -            size16s_t rndbit_128 =  fCAST8S_16S((1LL << (SHIFT - 1))); \
> -            size16s_t src_128 =  fCAST8S_16S(SRC); \
> -            size16s_t tmp128 = fADD128(src_128, rndbit_128);\
> +            rndbit_128 =  fCAST8S_16S((1LL << (SHIFT - 1))); \
> +            src_128 =  fCAST8S_16S(SRC); \
> +            tmp128 = fADD128(src_128, rndbit_128);\

Correct.

>               tmp128 = fSHIFTR128(tmp128, SHIFT);\
>               DST =  fCAST16S_8S(tmp128);\
>           }
> diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
> index a655634fd1..1ceb9453ee 100644
> --- a/target/hexagon/mmvec/macros.h
> +++ b/target/hexagon/mmvec/macros.h
> @@ -201,7 +201,7 @@
>       } while (0)
>   #define SCATTER_OP_WRITE_TO_MEM(TYPE) \
>       do { \
> -        uintptr_t ra = GETPC(); \
> +        ra = GETPC(); \

Hmm I'm not a big fan of having "public" macros (exposed in header)
which depend on local variable name. I'd rather rename the local 'ra'
variable.

That said, this macro is used twice, for 16/32bits, iterating on 8bits.
You could probably save some cycles using cpu_lduw_le_data_ra() /
cpu_ldl_be_data_ra(). If so, can be done later.

>           for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
>               if (test_bit(i, env->vtcm_log.mask)) { \
>                   TYPE dst = 0; \
> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 8ca3976a65..da10ac5847 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -132,10 +132,9 @@ void HELPER(gather_store)(CPUHexagonState *env, uint32_t addr, int slot)
>   void HELPER(commit_hvx_stores)(CPUHexagonState *env)
>   {
>       uintptr_t ra = GETPC();
> -    int i;
>   
>       /* Normal (possibly masked) vector store */
> -    for (i = 0; i < VSTORES_MAX; i++) {
> +    for (int i = 0; i < VSTORES_MAX; i++) {
>           if (env->vstore_pending[i]) {
>               env->vstore_pending[i] = 0;
>               target_ulong va = env->vstore[i].va;
> @@ -162,7 +161,7 @@ void HELPER(commit_hvx_stores)(CPUHexagonState *env)
>                   g_assert_not_reached();
>               }
>           } else {
> -            for (i = 0; i < sizeof(MMVector); i++) {
> +            for (int i = 0; i < sizeof(MMVector); i++) {
>                   if (test_bit(i, env->vtcm_log.mask)) {
>                       cpu_stb_data_ra(env, env->vtcm_log.va[i],
>                                       env->vtcm_log.data.ub[i], ra);

Correct.

> @@ -505,10 +504,8 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int args)
>   static void probe_hvx_stores(CPUHexagonState *env, int mmu_idx,
>                                       uintptr_t retaddr)
>   {
> -    int i;
> -
>       /* Normal (possibly masked) vector store */
> -    for (i = 0; i < VSTORES_MAX; i++) {
> +    for (int i = 0; i < VSTORES_MAX; i++) {

Correct.

>           if (env->vstore_pending[i]) {
>               target_ulong va = env->vstore[i].va;
>               int size = env->vstore[i].size;
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index c00254e4d5..a1c7cd6f21 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -553,7 +553,7 @@ static void gen_start_packet(DisasContext *ctx)
>       /* Preload the predicated registers into get_result_gpr(ctx, i) */
>       if (ctx->need_commit &&
>           !bitmap_empty(ctx->predicated_regs, TOTAL_PER_THREAD_REGS)) {
> -        int i = find_first_bit(ctx->predicated_regs, TOTAL_PER_THREAD_REGS);

Hmmm, matter of taste, in big functions where a very generic
variable is reused, reducing the scope seems safer (like you
did int commit_hvx_stores).

> +        i = find_first_bit(ctx->predicated_regs, TOTAL_PER_THREAD_REGS);
>           while (i < TOTAL_PER_THREAD_REGS) {
>               tcg_gen_mov_tl(get_result_gpr(ctx, i), hex_gpr[i]);
>               i = find_next_bit(ctx->predicated_regs, TOTAL_PER_THREAD_REGS,
> @@ -566,7 +566,7 @@ static void gen_start_packet(DisasContext *ctx)
>        * Only endloop instructions conditionally write to pred registers
>        */
>       if (ctx->need_commit && pkt->pkt_has_endloop) {
> -        for (int i = 0; i < ctx->preg_log_idx; i++) {
> +        for (i = 0; i < ctx->preg_log_idx; i++) {
>               int pred_num = ctx->preg_log[i];
>               ctx->new_pred_value[pred_num] = tcg_temp_new();
>               tcg_gen_mov_tl(ctx->new_pred_value[pred_num], hex_pred[pred_num]);

[...]

Preferably reworking SCATTER_OP_WRITE_TO_MEM:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Regards,

Phil.

Re: [PATCH 1/2] target/hexagon: move GETPC() calls to top level helpers

[Philippe Mathieu-Daudé]

On 4/10/23 14:39, Brian Cain wrote:
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> 
> As docs/devel/loads-stores.rst states:
> 
>    ``GETPC()`` should be used with great care: calling
>    it in other functions that are *not* the top level
>    ``HELPER(foo)`` will cause unexpected behavior. Instead, the
>    value of ``GETPC()`` should be read from the helper and passed
>    if needed to the functions that the helper calls.
> 
> Let's fix the GETPC() usage in Hexagon, making sure it's always called
> from top level helpers and passed down to the places where it's
> needed. There are a few snippets where that is not currently the case:
> 
> - probe_store(), which is only called from two helpers, so it's easy to
>    move GETPC() up.
> 
> - mem_load*() functions, which are also called directly from helpers,
>    but through the MEM_LOAD*() set of macros. Note that this are only
>    used when compiling with --disable-hexagon-idef-parser.
> 
>    In this case, we also take this opportunity to simplify the code,
>    unifying the mem_load*() functions.
> 
> - HELPER(probe_hvx_stores), when called from another helper, ends up
>    using its own GETPC() expansion instead of the top level caller.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> Message-Id: <2c74c3696946edba7cc5b2942cf296a5af532052.1689070412.git.quic_mathbern@quicinc.com>-ne

Odd '-ne' trailing.

> Reviewed-by: Brian Cain <bcain@quicinc.com>
> ---
>   target/hexagon/macros.h    | 19 +++++-----
>   target/hexagon/op_helper.c | 75 +++++++++++++++-----------------------
>   target/hexagon/op_helper.h |  9 -----
>   3 files changed, 38 insertions(+), 65 deletions(-)