On 7/5/23 23:17, ltaylorsimp...@gmail.com wrote:
-----Original Message-----
From: Matheus Tavares Bernardino <quic_mathb...@quicinc.com>
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 <quic_mathb...@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~
