-----Original Message-----
From: Matheus Tavares Bernardino <quic_mathb...@quicinc.com>
Sent: Monday, July 3, 2023 3:50 PM
To: qemu-devel@nongnu.org
Cc: bc...@quicinc.com; quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com
Subject: [PATCH] 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>
---
target/hexagon/macros.h | 22 ++++++++++-------
target/hexagon/op_helper.h | 11 ++------- target/hexagon/op_helper.c | 49
+++++++-------------------------------
3 files changed, 25 insertions(+), 57 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
5451b061ee..efb8013912 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
+
+#define MEM_LOADn(SIZE, VA) ({ \
+ check_noshuf(env, pkt_has_store_s1, slot, VA, SIZE); \
+ cpu_ldub_data_ra(env, VA, GETPC()); \
+})
Note that check_noshuf calls HELPER(probe_noshuf_load) and
HELPER(commit_store). Both of those call GETPC() from within. So, you'll
need to pull the contents into separate functions that take ra as an
argument.
Does this pass the test suite? You are only using the SIZE parameter in
check_noshuf, but cpu_ldub_data_ra only reads a single byte.
+#define MEM_LOAD1s(VA) ((int8_t)MEM_LOADn(1, VA)) #define
+MEM_LOAD1u(VA) ((uint8_t)MEM_LOADn(1, VA)) #define MEM_LOAD2s(VA)
+((int16_t)MEM_LOADn(2, VA)) #define MEM_LOAD2u(VA)
+((uint16_t)MEM_LOADn(2, VA)) #define MEM_LOAD4s(VA)
+((int32_t)MEM_LOADn(4, VA)) #define MEM_LOAD4u(VA)
+((uint32_t)MEM_LOADn(4, VA)) #define MEM_LOAD8s(VA)
+((int64_t)MEM_LOADn(8, VA)) #define MEM_LOAD8u(VA)
+((uint64_t)MEM_LOADn(8, VA))
A further cleanup would be to remove these altogether and modify the
definition of fLOAD to use MEM_LOADn.
#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) diff
--git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
8f3764d15e..845c3d197e 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size);
Does this really need to be non-static?
Thanks,
Taylor
