Re: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
On Thu, 24 Jun 2021 03:55:35 +
Taylor Simpson wrote:
> > +void gen_deposit_op(Context *c,
> > +YYLTYPE *locp,
> > +HexValue *dest,
> > +HexValue *value,
> > +HexValue *index,
> > +HexCast *cast)
>
> What's the difference between this and the gen_rdeposit_op above?
`gen_deposit_op` expects index and width (cast) to be immediates, while
`gen_rdeposit_op` does not.
We could merge them together, but it would just be a big "if" over the
whole function.
> > +HexValue gen_rextract_op(Context *c,
> > + YYLTYPE *locp,
> > + HexValue *source,
> > + int begin,
> > + int width) {
> > +
> > +HexValue gen_extract_op(Context *c,
> > +YYLTYPE *locp,
> > +HexValue *source,
> > +HexValue *index,
> > +HexExtract *extract) {
>
> What's the difference between this ant the gen_rextract_op above?
As before.
--
Alessandro Di Federico
rev.ng
Re: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
> > +void gen_setbits(Context *c, YYLTYPE *locp, HexValue *hi, HexValue *lo,
> > + HexValue *dst, HexValue *val)
> > +{
> > +yyassert(c, locp, hi->type == IMMEDIATE &&
> > + hi->imm.type == VALUE &&
> > + lo->type == IMMEDIATE &&
> > + lo->imm.type == VALUE,
> > + "Range deposit needs immediate values!\n");
> > +
> > +*val = rvalue_truncate(c, locp, val);
> > +unsigned len = hi->imm.value + 1 - lo->imm.value;
> > +HexValue tmp = gen_tmp(c, locp, 32);
> > +OUT(c, locp, "tcg_gen_neg_i32(", &tmp, ", ", val, ");\n");
> > +OUT(c, locp, "tcg_gen_deposit_i32(", dst, ", ", dst, ", ", &tmp, ", ");
> > +OUT(c, locp, lo, ", ", &len, ");\n");
>
>
> This doesn't match the C semantics of fSETBITS
>
> #define fSETBIT(N, DST, VAL) \
> do { \
> DST = (DST & ~(1ULL << (N))) | (((uint64_t)(VAL)) << (N)); \
> } while (0)
>
> #define fGETBIT(N, SRC) (((SRC) >> N) & 1)
> #define fSETBITS(HI, LO, DST, VAL) \
> do { \
> int j; \
> for (j = LO; j <= HI; j++) { \
> fSETBIT(j, DST, VAL); \
> } \
> } while (0)
>
> You need to put len copies of LSB(val), so emit something like this
> TCGv zero = tcg_const_tl(0);
> TCGv ones = tcg_const_tl(~0);
> tcg_gen_andi_tl(tmp, val, 1);
> tcg_gen_movcond_tl(TCG_COND_NE, tmp, tmp, zero, ones, zero);
> tcg_gen_deposit_tl(dst, dst, tmp, lo, len);
> tcg_temp_free(zero);
> tcg_temp_free(ones);
The change was suggested by (I think) Richard some patchset ago and I
think it is semantically equivalent.
I checked `compare.idef` and every value of `VAL` comes from a
comparison, which has value either `0` or `1`.
Applying `neg` turns it into either `0` or `0x`, making the
`deposit` work as intended.
Am I missing something?
Re: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
On Thu, 24 Jun 2021 03:55:35 +
Taylor Simpson wrote:
> > + | rvalue '[' rvalue ']'
> > + {
> > + @1.last_column = @4.last_column;
> > + if ($3.type == IMMEDIATE) {
> > + $$ = gen_tmp(c, &@1, $1.bit_width);
> > + OUT(c, &@1, "tcg_gen_extract_i", &$$.bit_width,
> > "(");
> > + OUT(c, &@1, &$$, ", ", &$1, ", ", &$3, ", 1);\n");
> > + } else {
> > + HexValue one = gen_imm_value(c, &@1, 1,
> > $3.bit_width);
> > + HexValue tmp = gen_bin_op(c, &@1, ASR_OP, &$1,
> > &$3);
> > + $$ = gen_bin_op(c, &@1, ANDB_OP, &tmp, &one);
>
> Can this be done with a tcg_gen_extract_tl or tcg_gen_sextract_tl?
Those require translation-time constants as offsets while we need TCGv:
tcg_gen_extract_i32(TCGv_i32 ret,
TCGv_i32 arg,
unsigned int ofs,
unsigned int len);
> Do you need to worry about signed-ness?
`gen_bin_op` should take care of that.
On Thu, 24 Jun 2021 03:55:35 +
Taylor Simpson wrote:
> > +void imm_print(Context *c, YYLTYPE *locp, HexImm *imm)
> > +{
> > +switch (imm->type) {
> > +case I:
> > +EMIT(c, "i");
> > +break;
> > +case VARIABLE:
> > +EMIT(c, "%ciV", imm->id);
> > +break;
> > +case VALUE:
> > +EMIT(c, "((int64_t)%" PRIu64 "ULL)", (int64_t)imm->value);
> > +break;
> > +case QEMU_TMP:
> > +EMIT(c, "qemu_tmp_%" PRIu64, imm->index);
> > +break;
> > +case IMM_PC:
> > +EMIT(c, "ctx->base.pc_next");
> > +break;
> > +case IMM_NPC:
> > +EMIT(c, "ctx->npc");
> > +break;
> > +case IMM_CONSTEXT:
> > +EMIT(c, "insn->extension_valid");
>
> The extension_valid field is a bool indicating if the instruction has
> a constant extender. Don't you want the actual value here?
No, this maps to Constant_extended:
#define fREAD_GP() (Constant_extended ? (0) : GP)
Constant extensions is handle for us externally.
On Thu, 24 Jun 2021 03:55:35 +
Taylor Simpson wrote:
> > +if (dest->bit_width != res.bit_width) {
> > +res = rvalue_truncate(c, locp, &res);
> > +}
> > +
> > +HexValue zero = gen_tmp_value(c, locp, "0", res.bit_width);
> > +OUT(c, locp, "tcg_gen_movcond_i", &res.bit_width,
> > "(TCG_COND_NE, ", dest);
> > +OUT(c, locp, ", ", &width_orig, ", ", &zero, ", ", &res, ", ",
> > dest,
> > +");\n");
> > +
> > +rvalue_free(c, locp, &zero);
> > +rvalue_free(c, locp, width);
> > +rvalue_free(c, locp, &res);
> > +}
> > +
> > +void gen_deposit_op(Context *c,
> > +YYLTYPE *locp,
> > +HexValue *dest,
> > +HexValue *value,
> > +HexValue *index,
> > +HexCast *cast)
>
> What's the difference between this and the gen_rdeposit_op above?
`gen_rdeposit_op` is general purpose, it takes start and width of the
deposit.
`gen_deposit_op` is more tailored to handle `fINSERT_BITS`.
We will improve `gen_rdeposit_op` in order to handle the immediate
case efficiently and then implement `gen_deposit_op` with
`gen_rdeposit_op`.
--
Alessandro Di Federico
rev.ng
RE: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
> -Original Message-
> From: Alessandro Di Federico
> Sent: Saturday, June 19, 2021 3:37 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson ; Brian Cain
> ; bab...@rev.ng; ni...@rev.ng; phi...@redhat.com;
> richard.hender...@linaro.org; Alessandro Di Federico
> Subject: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
>
> From: Paolo Montesel
>
> Signed-off-by: Alessandro Di Federico
> Signed-off-by: Paolo Montesel
> ---
> diff --git a/target/hexagon/idef-parser/idef-parser.y b/target/hexagon/idef-
> parser/idef-parser.y
> +for_statement : FOR '(' IMM '=' IMM ';' IMM '<' IMM ';' IMM PLUSPLUS ')'
> +{
> +@1.last_column = @13.last_column;
> +OUT(c, &@1, "for (int ", &$3, " = ", &$5, "; ",
> +&$7, " < ", &$9);
> +OUT(c, &@1, "; ", &$11, "++) {\n");
Increase indent level here
> +}
> +code_block
> +{
Decrease indent level
> +OUT(c, &@1, "}\n");
> +}
> + | FOR '(' IMM '=' IMM ';' IMM '<' IMM ';' IMM INC IMM ')'
> +{
> +@1.last_column = @14.last_column;
> +OUT(c, &@1, "for (int ", &$3, " = ", &$5, "; ",
> +&$7, " < ", &$9);
> +OUT(c, &@1, "; ", &$11, " += ", &$13, ") {\n");
Increase indent
> +}
> +code_block
> +{
Decrease indent
> +OUT(c, &@1, "}\n");
> +}
> + ;
> +
> +fpart1_statement : PART1
> + {
> + OUT(c, &@1, "if (insn->part1) {\n");
Increase indent
> + }
> + '(' statements ')'
> + {
> + @1.last_column = @3.last_column;
Emit the return first, then decrease indent before the curly
> + OUT(c, &@1, "return; }\n");
> + }
> + ;
> + | rvalue '[' rvalue ']'
> + {
> + @1.last_column = @4.last_column;
> + if ($3.type == IMMEDIATE) {
> + $$ = gen_tmp(c, &@1, $1.bit_width);
> + OUT(c, &@1, "tcg_gen_extract_i", &$$.bit_width, "(");
> + OUT(c, &@1, &$$, ", ", &$1, ", ", &$3, ", 1);\n");
> + } else {
> + HexValue one = gen_imm_value(c, &@1, 1, $3.bit_width);
> + HexValue tmp = gen_bin_op(c, &@1, ASR_OP, &$1, &$3);
> + $$ = gen_bin_op(c, &@1, ANDB_OP, &tmp, &one);
Can this be done with a tcg_gen_extract_tl or tcg_gen_sextract_tl?
Do you need to worry about signed-ness?
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> new file mode 100644
> +const char *creg_str[] = {"HEX_REG_SP", "HEX_REG_FP", "HEX_REG_LR",
> + "HEX_REG_GP", "HEX_REG_LC0", "HEX_REG_LC1",
> + "HEX_REG_SA0", "HEX_REG_SA1"};
SP, FP, LR shouldn't in this array.
> +void reg_compose(Context *c, YYLTYPE *locp, HexReg *reg, char reg_id[5])
> +{
> +switch (reg->type) {
> +case GENERAL_PURPOSE:
> +reg_id[0] = 'R';
> +break;
> +case CONTROL:
> +reg_id[0] = 'C';
> +break;
> +case MODIFIER:
> +reg_id[0] = 'M';
> +break;
> +case DOTNEW:
> +/* The DOTNEW case is managed by the upper level function */
Should we raise an error if we get here?
> +break;
> +}
> +switch (reg->bit_width) {
> +case 32:
> +reg_id[1] = reg->id;
> +reg_id[2] = 'V';
> +break;
> +case 64:
> +reg_id[1] = reg->id;
> +reg_id[2] = reg->id;
> +reg_id[3] = 'V';
> +break;
> +default:
> +yyassert(c, locp, false, "Unhandled register bit width!\n");
> +}
> +}
> +
> +void reg_print(Context *c, YYLTYPE *locp,
RE: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
> -Original Message- > From: Alessandro Di Federico > Sent: Saturday, June 19, 2021 3:37 AM > To: qemu-devel@nongnu.org > Cc: Taylor Simpson ; Brian Cain > ; bab...@rev.ng; ni...@rev.ng; phi...@redhat.com; > richard.hender...@linaro.org; Alessandro Di Federico > Subject: [PATCH v5 10/14] target/hexagon: import parser for idef-parser > > From: Paolo Montesel > > Signed-off-by: Alessandro Di Federico > Signed-off-by: Paolo Montesel > --- > diff --git a/target/hexagon/idef-parser/parser-helpers.h > b/target/hexagon/idef-parser/parser-helpers.h > new file mode 100644 > index 00..fec3ad7819 > --- /dev/null > +++ b/target/hexagon/idef-parser/parser-helpers.h > @@ -0,0 +1,347 @@ > + > +#define OUT_IMPL(c, locp, x)\ > +QEMU_GENERIC(typeof(*x),\ > +(char, str_print), \ > +(uint64_t, uint64_print), \ > +(int, int_print), \ > +(unsigned, uint_print),\ > +(HexValue, rvalue_out),\ > +out_assert \ > +)(c, locp, x); \ > + QEMU_GENERIC has been removed commit de51d8cbf0f9a9745ac02fb07e02063b7dfe35b9 Author: Richard Henderson Date: Mon Jun 14 16:31:42 2021 -0700 qemu/compiler: Remove QEMU_GENERIC All previous users now use C11 _Generic. Signed-off-by: Richard Henderson Reviewed-by: Alex Benne Message-Id: <20210614233143.1221879-8-richard.hender...@linaro.org> Signed-off-by: Paolo Bonzini You can now write this as #define OUT_IMPL(c, locp, x) \ _Generic(*x, \ char: str_print, \ uint64_t: uint64_print, \ int: int_print, \ unsigned: uint_print,\ HexValue: rvalue_out,\ default: out_assert \ )(c, locp, x); Thanks, Taylor
[PATCH v5 10/14] target/hexagon: import parser for idef-parser
From: Paolo Montesel
Signed-off-by: Alessandro Di Federico
Signed-off-by: Paolo Montesel
---
target/hexagon/idef-parser/idef-parser.y | 961 +++
target/hexagon/idef-parser/parser-helpers.c | 2396 +
target/hexagon/idef-parser/parser-helpers.h | 347 +++
target/hexagon/meson.build| 26 +-
tests/docker/dockerfiles/alpine.docker|1 +
tests/docker/dockerfiles/centos8.docker |1 +
tests/docker/dockerfiles/debian-amd64.docker |1 +
tests/docker/dockerfiles/debian10.docker |2 +
.../dockerfiles/fedora-i386-cross.docker |2 +
.../dockerfiles/fedora-win32-cross.docker |2 +
.../dockerfiles/fedora-win64-cross.docker |2 +
tests/docker/dockerfiles/fedora.docker|1 +
tests/docker/dockerfiles/opensuse-leap.docker |1 +
tests/docker/dockerfiles/ubuntu.docker|2 +
tests/docker/dockerfiles/ubuntu1804.docker|2 +
tests/docker/dockerfiles/ubuntu2004.docker|4 +-
16 files changed, 3749 insertions(+), 2 deletions(-)
create mode 100644 target/hexagon/idef-parser/idef-parser.y
create mode 100644 target/hexagon/idef-parser/parser-helpers.c
create mode 100644 target/hexagon/idef-parser/parser-helpers.h
diff --git a/target/hexagon/idef-parser/idef-parser.y
b/target/hexagon/idef-parser/idef-parser.y
new file mode 100644
index 00..5e4fbb6e75
--- /dev/null
+++ b/target/hexagon/idef-parser/idef-parser.y
@@ -0,0 +1,961 @@
+%{
+/*
+ * Copyright(c) 2019-2021 rev.ng Srls. All Rights Reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
