| Message ID | 93a2ca786530cbc8a94f7c7a6451f4f1f47c8a9b.1688581908.git.quic_mathbern@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Hexagon: move GETPC() calls to top level helpers | expand |
> -----Original Message----- > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > Sent: Wednesday, July 5, 2023 12:35 PM > To: qemu-devel@nongnu.org > Cc: quic_mathbern@quicinc.com; bcain@quicinc.com; > ltaylorsimpson@gmail.com; quic_mliebel@quicinc.com; > richard.henderson@linaro.org > Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > 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 two 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. > > Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > --- > v1: > d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathber > n@quicinc.com > > Changes in v2: > - Fixed wrong cpu_ld* unification from previous version. > - Passed retaddr down to check_noshuf() and further, as Taylor > suggested. > - Reorganized macros for simplification. > > target/hexagon/macros.h | 19 ++++++------ > target/hexagon/op_helper.h | 11 ++----- target/hexagon/op_helper.c | 62 > +++++++++++--------------------------- > 3 files changed, 29 insertions(+), 63 deletions(-) > > diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index > 5451b061ee..e44a932434 100644 > --- a/target/hexagon/macros.h > +++ b/target/hexagon/macros.h > @@ -173,15 +173,6 @@ > #define fLOAD(NUM, SIZE, SIGN, EA, DST) \ > - DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA) > + DST = (size##SIZE##SIGN##_t)({ \ > + check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \ > + MEM_LOAD##SIZE(env, EA, GETPC()); \ > + }) > #endif This should be formatted as #define fLOAD(...) \ do { \ check_noshuf(...); \ DST = ...; \ } while (0) > a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index > 8f3764d15e..7744e819ef 100644 > --- a/target/hexagon/op_helper.h > +++ b/target/hexagon/op_helper.h > +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, > + uint32_t slot, target_ulong vaddr, int size, > +uintptr_t ra); Are you sure this needs to be non-static? Othersiwe Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
On 7/5/23 23:17, ltaylorsimpson@gmail.com wrote: > > >> -----Original Message----- >> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> >> Sent: Wednesday, July 5, 2023 12:35 PM >> To: qemu-devel@nongnu.org >> Cc: quic_mathbern@quicinc.com; bcain@quicinc.com; >> ltaylorsimpson@gmail.com; quic_mliebel@quicinc.com; >> richard.henderson@linaro.org >> Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers >> >> 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 two 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. >> >> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> >> --- >> v1: >> d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathber >> n@quicinc.com >> >> Changes in v2: >> - Fixed wrong cpu_ld* unification from previous version. >> - Passed retaddr down to check_noshuf() and further, as Taylor >> suggested. >> - Reorganized macros for simplification. >> >> target/hexagon/macros.h | 19 ++++++------ >> target/hexagon/op_helper.h | 11 ++----- target/hexagon/op_helper.c | 62 >> +++++++++++--------------------------- >> 3 files changed, 29 insertions(+), 63 deletions(-) >> >> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index >> 5451b061ee..e44a932434 100644 >> --- a/target/hexagon/macros.h >> +++ b/target/hexagon/macros.h >> @@ -173,15 +173,6 @@ >> #define fLOAD(NUM, SIZE, SIGN, EA, DST) \ >> - DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA) >> + DST = (size##SIZE##SIGN##_t)({ \ >> + check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \ >> + MEM_LOAD##SIZE(env, EA, GETPC()); \ >> + }) >> #endif > > This should be formatted as > #define fLOAD(...) \ > do { \ > check_noshuf(...); \ > DST = ...; \ > } while (0) Alternately, DST = (size##SIZE##SIGN##_t)mem_load##SIZE(env, pkt_has_store_s1, slot, EA, GETPC()) and retain the mem_loadN functions with an extra argument. >> +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, >> + uint32_t slot, target_ulong vaddr, int size, >> +uintptr_t ra); > > Are you sure this needs to be non-static? I think only if the mem_loadN functions are retained may it be static. But perhaps the macro magic is actually limited to the same compilation unit? r~
> ltaylorsimpson@gmail.com wrote: > > > -----Original Message----- > > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > > Sent: Wednesday, July 5, 2023 12:35 PM > > To: qemu-devel@nongnu.org > > Cc: quic_mathbern@quicinc.com; bcain@quicinc.com; > > ltaylorsimpson@gmail.com; quic_mliebel@quicinc.com; > > richard.henderson@linaro.org > > Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index > > 5451b061ee..e44a932434 100644 > > --- a/target/hexagon/macros.h > > +++ b/target/hexagon/macros.h > > @@ -173,15 +173,6 @@ > > #define fLOAD(NUM, SIZE, SIGN, EA, DST) \ > > - DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA) > > + DST = (size##SIZE##SIGN##_t)({ \ > > + check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \ > > + MEM_LOAD##SIZE(env, EA, GETPC()); \ > > + }) > > #endif > > This should be formatted as > #define fLOAD(...) \ > do { \ > check_noshuf(...); \ > DST = ...; \ > } while (0) Ah, indeed, thanks! > > a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index > > 8f3764d15e..7744e819ef 100644 > > --- a/target/hexagon/op_helper.h > > +++ b/target/hexagon/op_helper.h > > +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, > > + uint32_t slot, target_ulong vaddr, int size, > > +uintptr_t ra); > > Are you sure this needs to be non-static? Yeah, since we remove the mem_load*() functions, check_noshuf() must now be visible to the other compilation units that include macros.h, as we will expand the fLOAD() macro to call it.
> -----Original Message----- > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > Sent: Thursday, July 6, 2023 5:23 AM > To: ltaylorsimpson@gmail.com > Cc: bcain@quicinc.com; qemu-devel@nongnu.org; > quic_mathbern@quicinc.com; quic_mliebel@quicinc.com; > richard.henderson@linaro.org > Subject: Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > ltaylorsimpson@gmail.com wrote: > > > > > -----Original Message----- > > > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > > > Sent: Wednesday, July 5, 2023 12:35 PM > > > To: qemu-devel@nongnu.org > > > Cc: quic_mathbern@quicinc.com; bcain@quicinc.com; > > > ltaylorsimpson@gmail.com; quic_mliebel@quicinc.com; > > > richard.henderson@linaro.org > > > Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > > > a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index > > > 8f3764d15e..7744e819ef 100644 > > > --- a/target/hexagon/op_helper.h > > > +++ b/target/hexagon/op_helper.h > > > +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, > > > + uint32_t slot, target_ulong vaddr, int size, > > > +uintptr_t ra); > > > > Are you sure this needs to be non-static? > > Yeah, since we remove the mem_load*() functions, check_noshuf() must > now be visible to the other compilation units that include macros.h, as we will > expand the fLOAD() macro to call it. Since the generated helper functions are included at the bottom of op_helper.c #include "helper_funcs_generated.c.inc" it can be static. It needs to be guarded with #ifndef CONFIG_HEXAGON_IDEF_PARSER because it is not used when by any of the idef-parser functions. Thanks, Taylor
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index 5451b061ee..e44a932434 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) + DST = (size##SIZE##SIGN##_t)({ \ + check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \ + MEM_LOAD##SIZE(env, EA, GETPC()); \ + }) #endif #define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE) diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index 8f3764d15e..7744e819ef 100644 --- a/target/hexagon/op_helper.h +++ b/target/hexagon/op_helper.h @@ -19,15 +19,8 @@ #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 check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, + uint32_t slot, target_ulong vaddr, int size, uintptr_t ra); void log_store64(CPUHexagonState *env, target_ulong addr, int64_t val, int width, int slot); void log_store32(CPUHexagonState *env, target_ulong addr, diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c index 12967ac21e..abc9fc4724 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,7 +498,8 @@ 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) @@ -547,12 +552,13 @@ 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); @@ -566,48 +572,16 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask) * If the load is in slot 0 and there is a store in slot1 (that * 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) +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, + 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); -} - /* Floating point */ float64 HELPER(conv_sf2df)(CPUHexagonState *env, float32 RsV) {
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 two 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. Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> --- v1: d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathbern@quicinc.com Changes in v2: - Fixed wrong cpu_ld* unification from previous version. - Passed retaddr down to check_noshuf() and further, as Taylor suggested. - Reorganized macros for simplification. target/hexagon/macros.h | 19 ++++++------ target/hexagon/op_helper.h | 11 ++----- target/hexagon/op_helper.c | 62 +++++++++++--------------------------- 3 files changed, 29 insertions(+), 63 deletions(-)