From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Brian Cain <bcain@quicinc.com>, qemu-devel@nongnu.org
Cc: armbru@redhat.com, richard.henderson@linaro.org,
peter.maydell@linaro.org, quic_mathbern@quicinc.com,
stefanha@redhat.com, ale@rev.ng, anjo@rev.ng,
quic_mliebel@quicinc.com, ltaylorsimpson@gmail.com
Subject: Re: [PATCH 2/2] target/hexagon: fix some occurrences of -Wshadow=local
Date: Thu, 5 Oct 2023 13:05:59 +0200 [thread overview]
Message-ID: <d0380a09-62e3-d296-6d3d-3ffbc2b9eca3@linaro.org> (raw)
In-Reply-To: <20231004123957.1732915-3-bcain@quicinc.com>
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.
next prev parent reply other threads:[~2023-10-05 11:07 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-04 12:39 [PATCH 0/2] Fix usage of GETPC(), variable shadowing Brian Cain
2023-10-04 12:39 ` [PATCH 1/2] target/hexagon: move GETPC() calls to top level helpers Brian Cain
2023-10-05 11:06 ` Philippe Mathieu-Daudé
2023-10-04 12:39 ` [PATCH 2/2] target/hexagon: fix some occurrences of -Wshadow=local Brian Cain
2023-10-05 11:05 ` Philippe Mathieu-Daudé [this message]
2023-10-04 15:11 ` [PATCH 0/2] Fix usage of GETPC(), variable shadowing Anton Johansson via
2023-10-04 15:11 ` Anton Johansson
2023-10-05 6:05 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0380a09-62e3-d296-6d3d-3ffbc2b9eca3@linaro.org \
--to=philmd@linaro.org \
--cc=ale@rev.ng \
--cc=anjo@rev.ng \
--cc=armbru@redhat.com \
--cc=bcain@quicinc.com \
--cc=ltaylorsimpson@gmail.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=quic_mathbern@quicinc.com \
--cc=quic_mliebel@quicinc.com \
--cc=richard.henderson@linaro.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).