All of lore.kernel.org
 help / color / mirror / Atom feed
From: Taylor Simpson <tsimpson@quicinc.com>
To: Alessandro Di Federico <ale.qemu@rev.ng>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: Alessandro Di Federico <ale@rev.ng>,
	Brian Cain <bcain@quicinc.com>,
	"richard.henderson@linaro.org" <richard.henderson@linaro.org>,
	"babush@rev.ng" <babush@rev.ng>, "nizzo@rev.ng" <nizzo@rev.ng>,
	"philmd@redhat.com" <philmd@redhat.com>
Subject: RE: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
Date: Thu, 24 Jun 2021 03:55:35 +0000	[thread overview]
Message-ID: <BYAPR02MB488679E9F94D484852DD2398DE079@BYAPR02MB4886.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20210619093713.1845446-11-ale.qemu@rev.ng>



> -----Original Message-----
> From: Alessandro Di Federico <ale.qemu@rev.ng>
> Sent: Saturday, June 19, 2021 3:37 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; babush@rev.ng; nizzo@rev.ng; philmd@redhat.com;
> richard.henderson@linaro.org; Alessandro Di Federico <ale@rev.ng>
> Subject: [PATCH v5 10/14] target/hexagon: import parser for idef-parser
> 
> From: Paolo Montesel <babush@rev.ng>
> 
> Signed-off-by: Alessandro Di Federico <ale@rev.ng>
> Signed-off-by: Paolo Montesel <babush@rev.ng>
> ---
> 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, HexReg *reg)
> +{
> +  if (reg->type == DOTNEW) {
> +    EMIT(c, "N%cN", reg->id);

Why not handle this in reg_compose?

> +  } else {
> +    char reg_id[5] = { 0 };
> +    reg_compose(c, locp, reg, reg_id);
> +    EMIT(c, "%s", reg_id);
> +  }
> +}
> +
> +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?

> +        break;


> +
> +static HexValue get_ternary_cond(Context *c, YYLTYPE *locp)
> +{
> +    yyassert(c, locp, is_inside_ternary(c), "unexisting condition");
> +    Ternary *t = &g_array_index(c->ternary, Ternary, 0);
> +    HexValue cond = t->cond;
> +    if (t->state == IN_RIGHT) {
> +        cond = gen_rvalue_notl(c, locp, &cond);
> +    }
> +    for (unsigned i = 1; i < c->ternary->len; ++i) {
> +        Ternary *right = &g_array_index(c->ternary, Ternary, i);
> +        HexValue other = right->cond;
> +        /* Invert condition if we are on the right side */
> +        if (right->state == IN_RIGHT) {
> +            other = gen_rvalue_notl(c, locp, &other);
> +        }
> +        cond = gen_bin_op(c, locp, ANDL_OP, &cond, &other);
> +    }
> +    return cond;
> +}
> +
> +/* Temporary values creation */
> +HexValue gen_tmp(Context *c, YYLTYPE *locp, int bit_width)
> +{
> +    HexValue rvalue;
> +    rvalue.type = TEMP;
> +    bit_width = (bit_width == 64) ? 64 : 32;

Better to assert it's either 64 or 32

> +    rvalue.bit_width = bit_width;
> +    rvalue.is_unsigned = false;
> +    rvalue.is_dotnew = false;
> +    rvalue.is_manual = false;
> +    rvalue.tmp.index = c->inst.tmp_count;
> +    OUT(c, locp, "TCGv_i", &bit_width, " tmp_", &c->inst.tmp_count,
> +        " = tcg_temp_new_i", &bit_width, "();\n");
> +    c->inst.tmp_count++;
> +    return rvalue;
> +}
> +


> +
> +void rvalue_free(Context *c, YYLTYPE *locp, HexValue *rvalue)

Should be called gen_rvalue_free

> +{
> +    if (rvalue->type == TEMP && !rvalue->is_manual) {
> +        const char *bit_suffix = (rvalue->bit_width == 64) ? "i64" : "i32";
> +        OUT(c, locp, "tcg_temp_free_", bit_suffix, "(", rvalue, ");\n");
> +    }
> +}


> +HexValue rvalue_extend(Context *c, YYLTYPE *locp, HexValue *rvalue)

Should be called gen_rvalue_extend

> +{
> +    if (rvalue->type == IMMEDIATE) {
> +        HexValue res = *rvalue;
> +        res.bit_width = 64;
> +        return res;
> +    } else {
> +        if (rvalue->bit_width == 32) {
> +            HexValue res = gen_tmp(c, locp, 64);
> +            const char *sign_suffix = (rvalue->is_unsigned) ? "u" : "";
> +            OUT(c, locp, "tcg_gen_ext", sign_suffix,
> +                "_i32_i64(", &res, ", ", rvalue, ");\n");
> +            rvalue_free(c, locp, rvalue);
> +            return res;
> +        }
> +    }
> +    return *rvalue;
> +}
> +
> +HexValue rvalue_truncate(Context *c, YYLTYPE *locp, HexValue *rvalue)

Should be called gen_rvalue_truncate

> +{
> +    if (rvalue->type == IMMEDIATE) {
> +        HexValue res = *rvalue;
> +        res.bit_width = 32;
> +        return res;
> +    } else {
> +        if (rvalue->bit_width == 64) {
> +            HexValue res = gen_tmp(c, locp, 32);
> +            OUT(c, locp, "tcg_gen_trunc_i64_tl(", &res, ", ", rvalue, ");\n");
> +            rvalue_free(c, locp, rvalue);
> +            return res;
> +        }
> +    }
> +    return *rvalue;
> +}
> +


> +void varid_allocate(Context *c,

gen_varid_allocate

> +                    YYLTYPE *locp,
> +                    HexValue *varid,
> +                    int width,
> +                    bool is_unsigned)
> +{
> +    varid->bit_width = width;
> +    const char *bit_suffix = width == 64 ? "64" : "32";
> +    int index = find_variable(c, locp, varid);
> +    bool found = index != -1;
> +    if (found) {
> +        Var *other = &g_array_index(c->inst.allocated, Var, index);
> +        varid->var.name = other->name;
> +        varid->bit_width = other->bit_width;
> +        varid->is_unsigned = other->is_unsigned;
> +    } else {
> +        EMIT_HEAD(c, "TCGv_i%s %s", bit_suffix, varid->var.name->str);
> +        EMIT_HEAD(c, " = tcg_temp_local_new_i%s();\n", bit_suffix);
> +        Var new_var = {
> +            .name = varid->var.name,
> +            .bit_width = width,
> +            .is_unsigned = is_unsigned,
> +        };
> +        g_array_append_val(c->inst.allocated, new_var);
> +    }
> +}
> +
> +void ea_free(Context *c, YYLTYPE *locp)

gen_ea_free

> +{
> +    OUT(c, locp, "tcg_temp_free(EA);\n");
> +}
> +HexValue gen_bin_cmp(Context *c,
> +                     YYLTYPE *locp,
> +                     TCGCond type,
> +                     HexValue *op1_ptr,
> +                     HexValue *op2_ptr)
> +{
> +    HexValue op1 = *op1_ptr;
> +    HexValue op2 = *op2_ptr;
> +    enum OpTypes op_types = (op1.type != IMMEDIATE) << 1
> +                            | (op2.type != IMMEDIATE);
> +
> +    /* Find bit width of the two operands, if at least one is 64 bit use a */
> +    /* 64bit operation, eventually extend 32bit operands. */

This is duplicated elsewhere (e.g., gen_bin_op) - should be pulled into a single function.

> +    bool op_is64bit = op1.bit_width == 64 || op2.bit_width == 64;
> +    const char *bit_suffix = op_is64bit ? "i64" : "i32";
> +    int bit_width = (op_is64bit) ? 64 : 32;
> +    if (op_is64bit) {
> +        switch (op_types) {
> +        case IMM_IMM:
> +            break;
> +        case IMM_REG:
> +            op2 = rvalue_extend(c, locp, &op2);
> +            break;
> +        case REG_IMM:
> +            op1 = rvalue_extend(c, locp, &op1);
> +            break;
> +        case REG_REG:
> +            op1 = rvalue_extend(c, locp, &op1);
> +            op2 = rvalue_extend(c, locp, &op2);
> +            break;
> +        }
> +    }



> +static void gen_mini_op(Context *c, YYLTYPE *locp, unsigned bit_width,
> +                        HexValue *res, enum OpTypes op_types,
> +                        HexValue *op1_ptr, HexValue *op2_ptr)
> +{
> +    HexValue op1 = *op1_ptr;
> +    HexValue op2 = *op2_ptr;
> +    const char *min = res->is_unsigned ? "tcg_gen_umin" : "tcg_gen_smin";
> +    switch (op_types) {
> +    case IMM_IMM:
> +        OUT(c, locp, "int", &bit_width, "_t ", res, " = (", &op1, " <= ");
> +        OUT(c, locp, &op2, ") ? ", &op1, " : ", &op2, ";\n");
> +        break;
> +    case IMM_REG:
> +        op1.bit_width = bit_width;
> +        op1 = rvalue_materialize(c, locp, &op1);
> +        OUT(c, locp, min, "_i", &bit_width, "(");
> +        OUT(c, locp, res, ", ", &op1, ", ", &op2, ");\n");
> +        break;
> +    case REG_IMM:
> +        op2.bit_width = bit_width;
> +        op2 = rvalue_materialize(c, locp, &op2);
> +        /* Fallthrough */
> +    case REG_REG:
> +        OUT(c, locp, min, "_i", &bit_width, "(");
> +        OUT(c, locp, res, ", ", &op1, ", ", &op2, ");\n");
> +        break;
> +    }
> +    rvalue_free(c, locp, &op1);
> +    rvalue_free(c, locp, &op2);
> +}
> +
> +static void gen_maxi_op(Context *c, YYLTYPE *locp, unsigned bit_width,
> +                        HexValue *res, enum OpTypes op_types,
> +                        HexValue *op1_ptr, HexValue *op2_ptr)
> +{
> +    HexValue op1 = *op1_ptr;
> +    HexValue op2 = *op2_ptr;
> +    const char *min = res->is_unsigned ? "tcg_gen_umax" : "tcg_gen_smax";
> +    switch (op_types) {
> +    case IMM_IMM:
> +        OUT(c, locp, "int", &bit_width, "_t ", res, " = (", &op1, " <= ");
> +        OUT(c, locp, &op2, ") ? ", &op2, " : ", &op1, ";\n");
> +        break;
> +    case IMM_REG:
> +        op1.bit_width = bit_width;
> +        op1 = rvalue_materialize(c, locp, &op1);
> +        OUT(c, locp, min, "_i", &bit_width, "(");
> +        OUT(c, locp, res, ", ", &op1, ", ", &op2, ");\n");
> +        break;
> +    case REG_IMM:
> +        op2.bit_width = bit_width;
> +        op2 = rvalue_materialize(c, locp, &op2);
> +        /* Fallthrough */
> +    case REG_REG:
> +        OUT(c, locp, min, "_i", &bit_width, "(");
> +        OUT(c, locp, res, ", ", &op1, ", ", &op2, ");\n");
> +        break;
> +    }
> +    rvalue_free(c, locp, &op1);
> +    rvalue_free(c, locp, &op2);
> +}

These two look basically the same, create a single function with one extra are indicating min/max.


> +HexValue gen_cast_op(Context *c,
> +                     YYLTYPE *locp,
> +                     HexValue *source,
> +                     unsigned target_width) {

Don't you need to worry about signed-ness of the result?

> +    if (source->bit_width == target_width) {
> +        return *source;
> +    } else if (source->type == IMMEDIATE) {
> +        HexValue res = *source;
> +        res.bit_width = target_width;
> +        return res;
> +    } else {
> +        HexValue res = gen_tmp(c, locp, target_width);
> +        /* Truncate */
> +        if (source->bit_width > target_width) {
> +            OUT(c, locp, "tcg_gen_trunc_i64_tl(", &res, ", ", source, ");\n");
> +        } else {
> +            if (source->is_unsigned) {
> +                /* Extend unsigned */
> +                OUT(c, locp, "tcg_gen_extu_i32_i64(",
> +                    &res, ", ", source, ");\n");
> +            } else {
> +                /* Extend signed */
> +                OUT(c, locp, "tcg_gen_ext_i32_i64(",
> +                    &res, ", ", source, ");\n");
> +            }
> +        }
> +        rvalue_free(c, locp, source);
> +        return res;
> +    }
> +}
> +
> +HexValue gen_extend_op(Context *c,
> +                       YYLTYPE *locp,
> +                       HexValue *src_width_ptr,
> +                       HexValue *dst_width_ptr,
> +                       HexValue *value_ptr,
> +                       bool is_unsigned) {
> +    HexValue src_width = *src_width_ptr;
> +    HexValue dst_width = *dst_width_ptr;
> +    HexValue value = *value_ptr;
> +    src_width = rvalue_extend(c, locp, &src_width);
> +    value = rvalue_extend(c, locp, &value);
> +    src_width = rvalue_materialize(c, locp, &src_width);
> +    value = rvalue_materialize(c, locp, &value);
> +
> +    HexValue res = gen_tmp(c, locp, 64);
> +    HexValue shift = gen_tmp_value(c, locp, "64", 64);
> +    HexValue zero = gen_tmp_value(c, locp, "0", 64);
> +    OUT(c, locp, "tcg_gen_sub_i64(",
> +        &shift, ", ", &shift, ", ", &src_width, ");\n");
> +    if (is_unsigned) {
> +        HexValue mask = gen_tmp_value(c, locp, "0xffffffffffffffff", 64);
> +        OUT(c, locp, "tcg_gen_shr_i64(",
> +            &mask, ", ", &mask, ", ", &shift, ");\n");
> +        OUT(c, locp, "tcg_gen_and_i64(",
> +            &res, ", ", &value, ", ", &mask, ");\n");
> +        rvalue_free(c, locp, &mask);

Can't you do this with tcg_gen_extract?

> +    } else {
> +        OUT(c, locp, "tcg_gen_shl_i64(",
> +            &res, ", ", &value, ", ", &shift, ");\n");
> +        OUT(c, locp, "tcg_gen_sar_i64(",
> +            &res, ", ", &res, ", ", &shift, ");\n");

Can't you do this with get_gen_sectract?

> +    }
> +    OUT(c, locp, "tcg_gen_movcond_i64(TCG_COND_EQ, ", &res, ", ");
> +    OUT(c, locp, &src_width, ", ", &zero, ", ", &zero, ", ", &res, ");\n");
> +
> +    rvalue_free(c, locp, &src_width);
> +    rvalue_free(c, locp, &dst_width);
> +    rvalue_free(c, locp, &value);
> +    rvalue_free(c, locp, &shift);
> +    rvalue_free(c, locp, &zero);
> +
> +    res.is_unsigned = is_unsigned;
> +    return res;
> +}
> +
> +void gen_rdeposit_op(Context *c,
> +                     YYLTYPE *locp,
> +                     HexValue *dest,
> +                     HexValue *value,
> +                     HexValue *begin,
> +                     HexValue *width)
> +{
> +    HexValue dest_m = *dest;
> +    dest_m.is_manual = true;
> +
> +    HexValue value_m = rvalue_extend(c, locp, value);
> +    HexValue begin_m = rvalue_extend(c, locp, begin);
> +    HexValue width_orig = *width;
> +    width_orig.is_manual = true;
> +    HexValue width_m = rvalue_extend(c, locp, &width_orig);
> +    width_m = rvalue_materialize(c, locp, &width_m);
> +
> +    HexValue mask = gen_tmp_value(c, locp, "0xffffffffffffffffUL", 64);
> +    mask.is_unsigned = true;
> +    HexValue k64 = gen_tmp_value(c, locp, "64", 64);
> +    k64 = gen_bin_op(c, locp, SUB_OP, &k64, &width_m);
> +    mask = gen_bin_op(c, locp, LSR_OP, &mask, &k64);
> +    begin_m.is_manual = true;
> +    mask = gen_bin_op(c, locp, ASL_OP, &mask, &begin_m);
> +    mask.is_manual = true;
> +    value_m = gen_bin_op(c, locp, ASL_OP, &value_m, &begin_m);
> +    value_m = gen_bin_op(c, locp, ANDB_OP, &value_m, &mask);
> +
> +    OUT(c, locp, "tcg_gen_not_i64(", &mask, ", ", &mask, ");\n");
> +    mask.is_manual = false;
> +    HexValue res = gen_bin_op(c, locp, ANDB_OP, &dest_m, &mask);
> +    res = gen_bin_op(c, locp, ORB_OP, &res, &value_m);
> +

Can't you do this with tcg_gen_deposit?

> +    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?


> +{
> +    yyassert(c, locp, index->type == IMMEDIATE,
> +             "Deposit index must be immediate!\n");
> +    HexValue value_m = *value;
> +    int bit_width = (dest->bit_width == 64) ? 64 : 32;
> +    int width = cast->bit_width;
> +    /* If the destination value is 32, truncate the value, otherwise extend */
> +    if (dest->bit_width != value->bit_width) {
> +        if (bit_width == 32) {
> +            value_m = rvalue_truncate(c, locp, &value_m);
> +        } else {
> +            value_m = rvalue_extend(c, locp, &value_m);
> +        }
> +    }
> +    value_m = rvalue_materialize(c, locp, &value_m);
> +    OUT(c, locp, "tcg_gen_deposit_i", &bit_width, "(", dest, ", ", dest, ", ");
> +    OUT(c, locp, &value_m, ", ", index, " * ", &width, ", ", &width, ");\n");
> +    rvalue_free(c, locp, index);
> +    rvalue_free(c, locp, &value_m);
> +}
> +
> +HexValue gen_rextract_op(Context *c,
> +                         YYLTYPE *locp,
> +                         HexValue *source,
> +                         int begin,
> +                         int width) {
> +    int bit_width = (source->bit_width == 64) ? 64 : 32;
> +    HexValue res = gen_tmp(c, locp, bit_width);
> +    OUT(c, locp, "tcg_gen_extract_i", &bit_width, "(", &res);
> +    OUT(c, locp, ", ", source, ", ", &begin, ", ", &width, ");\n");
> +    rvalue_free(c, locp, source);
> +    return res;
> +}
> +
> +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?

> +    yyassert(c, locp, index->type == IMMEDIATE,
> +             "Extract index must be immediate!\n");
> +    int bit_width = (source->bit_width == 64) ? 64 : 32;
> +    const char *sign_prefix = (extract->is_unsigned) ? "" : "s";
> +    int width = extract->bit_width;
> +    HexValue res = gen_tmp(c, locp, bit_width);
> +    res.is_unsigned = extract->is_unsigned;
> +    OUT(c, locp, "tcg_gen_", sign_prefix, "extract_i", &bit_width,
> +        "(", &res, ", ", source);
> +    OUT(c, locp, ", ", index, " * ", &width, ", ", &width, ");\n");
> +
> +    /* Some extract operations have bit_width != storage_bit_width */
> +    if (extract->storage_bit_width > bit_width) {
> +        HexValue tmp = gen_tmp(c, locp, extract->storage_bit_width);
> +        tmp.is_unsigned = extract->is_unsigned;
> +        if (extract->is_unsigned) {
> +            /* Extend unsigned */
> +            OUT(c, locp, "tcg_gen_extu_i32_i64(",
> +                &tmp, ", ", &res, ");\n");
> +        } else {
> +            /* Extend signed */
> +            OUT(c, locp, "tcg_gen_ext_i32_i64(",
> +                &tmp, ", ", &res, ");\n");
> +        }
> +        rvalue_free(c, locp, &res);
> +        res = tmp;
> +    }
> +
> +    rvalue_free(c, locp, source);
> +    rvalue_free(c, locp, index);
> +    return res;
> +}
> +
> +HexValue gen_read_creg(Context *c, YYLTYPE *locp, HexValue *reg)
> +{
> +    yyassert(c, locp, reg->type == REGISTER, "reg must be a register!");
> +    if (reg->reg.id < 'a') {

What is this check telling us?

> +        HexValue tmp = gen_tmp_value(c, locp, "0", 32);
> +        const char *id = creg_str[(uint8_t)reg->reg.id];
> +        OUT(c, locp, "READ_REG(", &tmp, ", ", id, ");\n");

Change READ_REG to gen_read_reg - that's what the macro is.

> +        rvalue_free(c, locp, reg);
> +        return tmp;
> +    }
> +    return *reg;
> +}
> +


> +/* Circular addressing mode with auto-increment */
> +void gen_circ_op(Context *c,
> +                 YYLTYPE *locp,
> +                 HexValue *addr,
> +                 HexValue *increment,
> +                 HexValue *modifier) {
> +    HexValue increment_m = *increment;
> +    HexValue cs = gen_tmp(c, locp, 32);
> +    increment_m = rvalue_materialize(c, locp, &increment_m);
> +    OUT(c, locp, "READ_REG(", &cs, ", HEX_REG_CS0 + MuN);\n");

Change READ_REG to gen_read_reg

> +    OUT(c,
> +        locp,
> +        "gen_helper_fcircadd(",
> +        addr,
> +        ", ",
> +        addr,
> +        ", ",
> +        &increment_m,
> +        ", ",
> +        modifier);
> +    OUT(c, locp, ", ", &cs, ");\n");
> +    rvalue_free(c, locp, &increment_m);
> +    rvalue_free(c, locp, modifier);
> +    rvalue_free(c, locp, &cs);
> +}



> +void gen_load(Context *c, YYLTYPE *locp, HexValue *num, HexValue *size,
> +              bool is_unsigned, HexValue *ea, HexValue *dst)
> +{
> +    /* Memop width is specified in the load macro */
> +    int bit_width = (size->imm.value > 4) ? 64 : 32;
> +    const char *sign_suffix = (size->imm.value > 4)
> +                              ? ""
> +                              : ((is_unsigned) ? "u" : "s");
> +    char size_suffix[4] = { 0 };
> +    /* Create temporary variable (if not present) */
> +    if (dst->type == VARID) {
> +        /* TODO: this is a common pattern, the parser should be varid-aware.
> */
> +        varid_allocate(c, locp, dst, bit_width, is_unsigned);
> +    }
> +    snprintf(size_suffix, 4, "%" PRIu64, size->imm.value * 8);
> +    if (bit_width == 32) {
> +        *dst = rvalue_truncate(c, locp, dst);
> +    } else {
> +        *dst = rvalue_extend(c, locp, dst);
> +    }

Why is the truncate/extend needed for the destination?

> +    int var_id = find_variable(c, locp, ea);
> +    yyassert(c, locp, var_id != -1, "Load variable must exist!\n");
> +    /* We need to enforce the variable size */
> +    ea->bit_width = g_array_index(c->inst.allocated, Var, var_id).bit_width;
> +    if (ea->bit_width != 32) {
> +        *ea = rvalue_truncate(c, locp, ea);
> +    }
> +    OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_store_s1) {\n");
> +    OUT(c, locp, "process_store(ctx, pkt, 1);\n");

Indent

> +    OUT(c, locp, "}\n");
> +    OUT(c, locp, "tcg_gen_qemu_ld", size_suffix, sign_suffix);
> +    OUT(c, locp, "(", dst, ", ", ea, ", 0);\n");
> +    /* If the var in EA was truncated it is now a tmp HexValue, so free it. */
> +    rvalue_free(c, locp, ea);
> +}
> +
> +void gen_store(Context *c, YYLTYPE *locp, HexValue *num, HexValue
> *size,
> +               HexValue *ea, HexValue *src)
> +{
> +    /* Memop width is specified in the store macro */
> +    int mem_width = size->imm.value;
> +    /* Adjust operand bit width to memop bit width */
> +    if (mem_width < 8) {
> +        *src = rvalue_truncate(c, locp, src);
> +    } else {
> +        *src = rvalue_extend(c, locp, src);
> +    }

Why is this needed?

> +    assert(ea->type == VARID);
> +    int var_id = find_variable(c, locp, ea);
> +    yyassert(c, locp, var_id != -1, "Load variable must exist!\n");
> +    /* We need to enforce the variable size */
> +    ea->bit_width = g_array_index(c->inst.allocated, Var, var_id).bit_width;
> +    if (ea->bit_width != 32) {
> +        *ea = rvalue_truncate(c, locp, ea);
> +    }

How can ea be not 32 bits?

> +    *src = rvalue_materialize(c, locp, src);
> +    OUT(c, locp, "gen_store", &mem_width, "(cpu_env, ", ea, ", ", src);
> +    OUT(c, locp, ", ctx, insn->slot);\n");
> +    rvalue_free(c, locp, src);
> +    /* If the var in ea was truncated it is now a tmp HexValue, so free it. */
> +    rvalue_free(c, locp, ea);
> +}
> +


> +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);



> +HexValue gen_rvalue_pow(Context *c, YYLTYPE *locp, HexValue *l,
> HexValue *r)

Which instruction calls this?  I don't think there is one.  If not, remove the POW token from the lexer and the associated rules from the parser.



> +HexValue gen_rvalue_abs(Context *c, YYLTYPE *locp, HexValue *v)
> +{
> +    const char *bit_suffix = (v->bit_width == 64) ? "i64" : "i32";
> +    int bit_width = (v->bit_width == 64) ? 64 : 32;
> +    HexValue res;
> +    res.is_unsigned = v->is_unsigned;
> +    res.is_dotnew = false;
> +    res.is_manual = false;
> +    if (v->type == IMMEDIATE) {
> +        res.type = IMMEDIATE;
> +        res.imm.type = QEMU_TMP;
> +        res.imm.index = c->inst.qemu_tmp_count;
> +        OUT(c, locp, "int", &bit_width, "_t ", &res, " = abs(", v, ");\n");
> +        c->inst.qemu_tmp_count++;
> +    } else {
> +        res = gen_tmp(c, locp, bit_width);
> +        HexValue zero = gen_tmp_value(c, locp, "0", bit_width);
> +        OUT(c, locp, "tcg_gen_neg_", bit_suffix, "(", &res, ", ", v, ");\n");
> +        OUT(c, locp, "tcg_gen_movcond_i", &bit_width);
> +        OUT(c, locp, "(TCG_COND_GT, ", &res, ", ", v, ", ", &zero);

tcg_gen_abs_i<bit_width>

> +        OUT(c, locp, ", ", v, ", ", &res, ");\n");
> +        rvalue_free(c, locp, &zero);
> +        rvalue_free(c, locp, v);
> +    }
> +    return res;
> +}
> +
> +HexValue gen_rvalue_brev(Context *c, YYLTYPE *locp, HexValue *v)
> +{
> +    yyassert(c, locp, v->bit_width <= 32,
> +             "fbrev not implemented for 64-bit integers!");
> +    HexValue res = gen_tmp(c, locp, v->bit_width);
> +    *v = rvalue_materialize(c, locp, v);
> +    OUT(c, locp, "gen_fbrev(", &res, ", ", v, ");\n");

gen_helper_fbrev



> diff --git a/target/hexagon/idef-parser/parser-helpers.h
> b/target/hexagon/idef-parser/parser-helpers.h
> new file mode 100644

> +#define OUT(c, locp, ...) FOR_EACH((c), (locp), OUT_IMPL, __VA_ARGS__)

You should be able to handle indenting here.  Unfortunately, many of the C statements generated use multiple OUT invocations.
Create two macros
	OUT			prints indentation, then the text		used for beginning a line of output
              OUT_CONTINUE	just print the text				used for continuing a line

> diff --git a/target/hexagon/meson.build b/target/hexagon/meson.build
> index 329219463f..a2257d41a5 100644
> --- a/target/hexagon/meson.build
> +++ b/target/hexagon/meson.build
> @@ -183,7 +183,7 @@ idef_parser_input_generated = custom_target(
>      command: [python, files('gen_idef_parser_funcs.py'),
> semantics_generated, attribs_def, gen_tcg_h, '@OUTPUT@'],
>  )
> 
> -idef_parser_input_generated_prep = custom_target(
> +preprocessed_idef_parser_input_generated = custom_target(

Don't change the name of this here, use the name you want in the patch where it was introduced.



  parent reply	other threads:[~2021-06-24  3:56 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-19  9:36 [PATCH v5 00/14] target/hexagon: introduce idef-parser Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 01/14] tcg: expose TCGCond manipulation routines Alessandro Di Federico via
2021-06-19 13:51   ` Richard Henderson
2021-06-19  9:37 ` [PATCH v5 02/14] target/hexagon: update MAINTAINERS for idef-parser Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 03/14] target/hexagon: import README " Alessandro Di Federico via
2021-06-23 15:46   ` Taylor Simpson
2021-06-24 13:51     ` Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 04/14] target/hexagon: make slot number an unsigned Alessandro Di Federico via
2021-06-23 15:58   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 05/14] target/hexagon: make helper functions non-static Alessandro Di Federico via
2021-06-23 18:29   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 06/14] target/hexagon: introduce new helper functions Alessandro Di Federico via
2021-06-23 12:05   ` Taylor Simpson
2021-06-23 18:49   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 07/14] target/hexagon: expose next PC in DisasContext Alessandro Di Federico via
2021-06-23 18:54   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 08/14] target/hexagon: prepare input for the idef-parser Alessandro Di Federico via
2021-06-23 19:37   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 09/14] target/hexagon: import lexer for idef-parser Alessandro Di Federico via
2021-06-23 20:05   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 10/14] target/hexagon: import parser " Alessandro Di Federico via
2021-06-22 22:35   ` Taylor Simpson
2021-06-24  3:55   ` Taylor Simpson [this message]
2021-06-29 14:26     ` Alessandro Di Federico via
2021-06-30 16:51     ` Paolo Montesel
2021-07-05 16:47     ` Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 11/14] target/hexagon: call idef-parser functions Alessandro Di Federico via
2021-06-25 22:00   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 12/14] target/hexagon: remove unused macros and functions Alessandro Di Federico via
2021-06-25 22:02   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 13/14] target/hexagon: import additional tests Alessandro Di Federico via
2021-06-25 23:56   ` Taylor Simpson
2021-06-28 22:39     ` Taylor Simpson
2021-07-05 16:50     ` Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 14/14] gitlab-ci: do not use qemu-project Docker registry Alessandro Di Federico via
2021-06-29 14:26   ` Alessandro Di Federico via
2021-06-29 14:37   ` Daniel P. Berrangé
2021-07-08 16:00     ` Alessandro Di Federico via

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR02MB488679E9F94D484852DD2398DE079@BYAPR02MB4886.namprd02.prod.outlook.com \
    --to=tsimpson@quicinc.com \
    --cc=ale.qemu@rev.ng \
    --cc=ale@rev.ng \
    --cc=babush@rev.ng \
    --cc=bcain@quicinc.com \
    --cc=nizzo@rev.ng \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.