RE: [PATCH v6 07/12] target/hexagon: prepare input for the idef-parser

From: Taylor Simpson <tsimpson@quicinc.com>
To: Alessandro Di Federico <ale.qemu@rev.ng>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "babush@rev.ng" <babush@rev.ng>, Brian Cain <bcain@quicinc.com>,
	"richard.henderson@linaro.org" <richard.henderson@linaro.org>,
	"nizzo@rev.ng" <nizzo@rev.ng>,
	Alessandro Di Federico <ale@rev.ng>
Subject: RE: [PATCH v6 07/12] target/hexagon: prepare input for the idef-parser
Date: Tue, 7 Sep 2021 18:09:01 +0000	[thread overview]
Message-ID: <BYAPR02MB48869CA040DC32F2244A1952DED39@BYAPR02MB4886.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20210720123031.1101682-8-ale.qemu@rev.ng>

> -----Original Message-----
> From: Alessandro Di Federico <ale.qemu@rev.ng>
> Sent: Tuesday, July 20, 2021 7:30 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; babush@rev.ng; nizzo@rev.ng;
> richard.henderson@linaro.org; Alessandro Di Federico <ale@rev.ng>
> Subject: [PATCH v6 07/12] target/hexagon: prepare input for the idef-parser
> 
> From: Alessandro Di Federico <ale@rev.ng>
> 
> Introduce infrastructure necessary to produce a file suitable for being parsed
> by the idef-parser.
> 
> Signed-off-by: Alessandro Di Federico <ale@rev.ng>

> diff --git a/target/hexagon/gen_idef_parser_funcs.py
> b/target/hexagon/gen_idef_parser_funcs.py
> new file mode 100644
> index 0000000000..7b8e0f6981
> --- /dev/null
> +++ b/target/hexagon/gen_idef_parser_funcs.py
> +def main():
> +    hex_common.read_semantics_file(sys.argv[1])
> +    hex_common.read_attribs_file(sys.argv[2])
> +    hex_common.read_overrides_file(sys.argv[3])

Do you really need the overrides file?

> diff --git a/target/hexagon/idef-parser/macros.inc b/target/hexagon/idef-
> parser/macros.inc
> new file mode 100644
> index 0000000000..20535691e8
> --- /dev/null
> +++ b/target/hexagon/idef-parser/macros.inc

> +/* Ease parsing */

> +#define fADDSAT64(DST, A, B)                                            \
> +    __a = fCAST8u(A);                                                   \
> +    __b = fCAST8u(B);                                                   \
> +    __sum = __a + __b;                                                  \
> +    __xor = __a ^ __b;                                                  \
> +    __mask = fCAST8s(0x8000000000000000ULL);                            \
> +    if (((__a ^ __b) | ~(__a ^ __sum)) & __mask) {                      \
> +        DST = __sum;                                                    \
> +    } else {                                                            \
> +        DST = ((__sum & __mask) >> 63) + __mask;                        \
> +        fSET_OVERFLOW();                                                \
> +    }

There are a bunch of these with pretty complex semantics.  Wouldn't it be easier to recognize them in the parser and call a hand-written helper?

> +/* Assignments */
> +#define fPCALIGN(IMM) (IMM = IMM & ~3)
> +#define fWRITE_LR(A) (LR = A)
> +#define fWRITE_FP(A) (FP = A)
> +#define fWRITE_SP(A) (SP = A)
> +#define fBRANCH(LOC, TYPE) (PC = LOC)
> +#define fJUMPR(REGNO, TARGET, TYPE) (PC = TARGET)

This should invoke write_new_pc because there may be more than one in the same packet.

> +fWRITE_NPC(VAL) (PC = VAL)

Ditto

> +/* Binary operators */
> +#define fADD128(A, B) (A + B)
> +#define fSUB128(A, B) (A - B)
> +#define fSHIFTR128(A, B) (size8s_t) (A >> B)
> +#define fSHIFTL128(A, B) (A << B)
> +#define fAND128(A, B) (A & B)

Will these operate on 128 bit numbers?