Re: [PATCH 12/38] target/hexagon: Add imported macro, attr defs for sysemu

[Brian Cain]

On 3/7/2025 1:01 PM, ltaylorsimp...@gmail.com wrote:

-----Original Message-----
From: Brian Cain <brian.c...@oss.qualcomm.com>
Sent: Friday, February 28, 2025 11:26 PM
To: qemu-devel@nongnu.org
Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
alex.ben...@linaro.org; quic_mbur...@quicinc.com;
sidn...@quicinc.com; Brian Cain <bc...@quicinc.com>
Subject: [PATCH 12/38] target/hexagon: Add imported macro, attr defs for
sysemu

From: Brian Cain <bc...@quicinc.com>

Signed-off-by: Brian Cain <brian.c...@oss.qualcomm.com>
---
  target/hexagon/attribs_def.h.inc   | 414 +++++++++++++++++++--
  target/hexagon/imported/macros.def | 558
+++++++++++++++++++++++++++++
  2 files changed, 942 insertions(+), 30 deletions(-)  mode change 100755 =>
100644 target/hexagon/imported/macros.def

diff --git a/target/hexagon/attribs_def.h.inc
b/target/hexagon/attribs_def.h.inc
index 9e3a05f882..e6523a739b 100644
--- a/target/hexagon/attribs_def.h.inc
+++ b/target/hexagon/attribs_def.h.inc
@@ -19,20 +19,41 @@
  DEF_ATTRIB(AA_DUMMY, "Dummy Zeroth Attribute", "", "")

  /* Misc */
+DEF_ATTRIB(FAKEINSN, "Not a real instruction", "", "")
+DEF_ATTRIB(MAPPING, "Not real -- asm mapped", "", "")
+DEF_ATTRIB(CONDMAPPING, "Not real -- mapped based on values", "", "")
  DEF_ATTRIB(EXTENSION, "Extension instruction", "", "")
+DEF_ATTRIB(SHARED_EXTENSION, "Shared extension instruction", "", "")
+DEF_ATTRIB(CABAC,
+           "Cabac Instruction. Used in conjuction with QDSP6_CABAC_PRESENT",
"",
+           "")
+DEF_ATTRIB(EXPERIMENTAL, "This may not work correctly not supported by
RTL.",
+           "", "")
Personally, I don't think we should be adding all of these.  Few are needed, 
and we run the risk of having attributes that aren’t used in QEMU and therefore 
aren’t properly implemented in QEMU.  Somewhere down the road, an instruction 
or macro could show up in the imported directory with such an attribute, and it 
will cause unnecessary headaches.  Examples above are CONDMAPPING and 
EXPERIMENTAL.  These should be included in hex_common.tag_ignore.

Better to wait until an instruction in a future version of Hexagon shows up 
that uses an attribute.  These will be few, so it will be simpler to examine 
each new attribute to ensure it is properly implemented in QEMU.

  /* access to implicit registers */
  DEF_ATTRIB(IMPLICIT_WRITES_LR, "Writes the link register", "", "UREG.LR")
+DEF_ATTRIB(IMPLICIT_READS_LR, "Reads the link register", "UREG.LR", "")
+DEF_ATTRIB(IMPLICIT_READS_LC0, "Reads loop count for loop 0",
+"UREG.LC0", "") DEF_ATTRIB(IMPLICIT_READS_LC1, "Reads loop count for
+loop 1", "UREG.LC1", "") DEF_ATTRIB(IMPLICIT_READS_SA0, "Reads start
+address for loop 0", "UREG.SA0", "") DEF_ATTRIB(IMPLICIT_READS_SA1,
+"Reads start address for loop 1", "UREG.SA1", "")
+DEF_ATTRIB(IMPLICIT_WRITES_PC, "Writes the program counter", "",
+"UREG.PC") DEF_ATTRIB(IMPLICIT_READS_PC, "Reads the program
counter",
+"UREG.PC", "")
  DEF_ATTRIB(IMPLICIT_WRITES_SP, "Writes the stack pointer", "",
"UREG.SP")
+DEF_ATTRIB(IMPLICIT_READS_SP, "Reads the stack pointer", "UREG.SP",
"")
  DEF_ATTRIB(IMPLICIT_WRITES_FP, "Writes the frame pointer", "",
"UREG.FP")
+DEF_ATTRIB(IMPLICIT_READS_FP, "Reads the frame pointer", "UREG.FP",
"")
+DEF_ATTRIB(IMPLICIT_WRITES_GP, "Writes the GP register", "",
"UREG.GP")
+DEF_ATTRIB(IMPLICIT_READS_GP, "Reads the GP register", "UREG.GP", "")
  DEF_ATTRIB(IMPLICIT_WRITES_LC0, "Writes loop count for loop 0", "",
"UREG.LC0")  DEF_ATTRIB(IMPLICIT_WRITES_LC1, "Writes loop count for
loop 1", "", "UREG.LC1")  DEF_ATTRIB(IMPLICIT_WRITES_SA0, "Writes start
addr for loop 0", "", "UREG.SA0")  DEF_ATTRIB(IMPLICIT_WRITES_SA1,
"Writes start addr for loop 1", "", "UREG.SA1")
+DEF_ATTRIB(IMPLICIT_WRITES_R00, "Writes Register 0", "", "UREG.R00")
The IMPLICIT_READS_* and IMPLICIT_WRITES_* are examples that would need to be 
handled properly if ever used.  Look at IMPLICIT_*_P0 to see how they are used 
in translate.c::analyze_packet.  Imagine a day in the future when an 
instruction gets imported with IMPLICIT_WRITES_R00 attribute.  When that 
instruction is in a packet with an instruction that reads R0, analyze_packet 
will not know there is a conflict and decide it's OK to short-circuit the 
packet semantics.  That bug would go unnoticed for a long time and only show up 
when a large program runs incorrectly on QEMU.

Our verification strategy includes using a tool to generate random 
packets and execute those under a debugger. comparing the register, 
memory state with hexagon-sim -- like risu does IIRC. This means that 
bugs like this are much easier to find (as long as they result in a 
change to architectural state visible from the gdbstub).  This tool 
isn't public yet but should be within ~weeks/months.


Nevertheless, I think it's worth considering this advice so I'll explore 
addressing this for v3.


Thanks,
Taylor