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

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com; Brian Cain <bcain@quicinc.com>
> Subject: [PATCH 12/38] target/hexagon: Add imported macro, attr defs for
> sysemu
> 
> From: Brian Cain <bcain@quicinc.com>
> 
> Signed-off-by: Brian Cain <brian.cain@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.

Thanks,
Taylor