RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Thursday, July 6, 2023 5:23 AM > To: ltaylorsimp...@gmail.com > Cc: bc...@quicinc.com; qemu-devel@nongnu.org; > quic_mathb...@quicinc.com; quic_mlie...@quicinc.com; > richard.hender...@linaro.org > Subject: Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > ltaylorsimp...@gmail.com wrote: > > > > > -Original Message- > > > From: Matheus Tavares Bernardino > > > Sent: Wednesday, July 5, 2023 12:35 PM > > > To: qemu-devel@nongnu.org > > > Cc: quic_mathb...@quicinc.com; bc...@quicinc.com; > > > ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com; > > > richard.hender...@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
Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> ltaylorsimp...@gmail.com wrote:
>
> > -Original Message-
> > From: Matheus Tavares Bernardino
> > Sent: Wednesday, July 5, 2023 12:35 PM
> > To: qemu-devel@nongnu.org
> > Cc: quic_mathb...@quicinc.com; bc...@quicinc.com;
> > ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com;
> > richard.hender...@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.
Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
On 7/5/23 23:17, ltaylorsimp...@gmail.com wrote:
-Original Message-
From: Matheus Tavares Bernardino
Sent: Wednesday, July 5, 2023 12:35 PM
To: qemu-devel@nongnu.org
Cc: quic_mathb...@quicinc.com; bc...@quicinc.com;
ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com;
richard.hender...@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
---
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~
RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Wednesday, July 5, 2023 12:35 PM
> To: qemu-devel@nongnu.org
> Cc: quic_mathb...@quicinc.com; bc...@quicinc.com;
> ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com;
> richard.hender...@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
> ---
> 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
