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.