RE: [PATCH v6 09/12] target/hexagon: import parser for idef-parser

[Taylor Simpson]

> -----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 09/12] 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/parser-helpers.h
> b/target/hexagon/idef-parser/parser-helpers.h
> +#define OUT_IMPL(c, locp, x)                    \
> +    _Generic(*x,                                \
> +        char:      str_print,                   \
> +        uint8_t:   uint8_print,                 \
> +        uint64_t:  uint64_print,                \
> +        int:       int_print,                   \
> +        unsigned:  uint_print,                  \
> +        HexValue:  rvalue_out,                  \
> +        default:   out_assert                   \
> +    )(c, locp, x);

Unless something has changed, qemu requires building with GCC 4.8 which is old 
enough that it doesn't support C11 _Generic.


> +HexValue gen_bin_cmp(Context *c,
> +                     YYLTYPE *locp,
> +                     TCGCond type,
> +                     HexValue *op1_ptr,
> +                     HexValue *op2_ptr);
> +
> +/* Code generation functions */

Move this comment above the previous function

> +HexValue gen_bin_op(Context *c,
> +                       YYLTYPE *locp,
> +                       OpType type,
> +                       HexValue *operand1,
> +                       HexValue *operand2);
> +
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> new file mode 100644

> +void pre_print(Context *c, YYLTYPE *locp, HexPre *pre, bool is_dotnew)
> +{
> +    (void) locp;
> +    char suffix = is_dotnew ? 'N' : 'V';
> +    EMIT(c, "P%c%c", pre->id, suffix);
> +}

Call this pred_print

> +
> +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:
> +        reg_id[0] = 'N';
> +        reg_id[1] = reg->id;
> +        reg_id[2] = 'N';
> +        return;
> +    }
> +    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");
> +    }
> +}

It would be better to zero out the result in the above function than having the 
caller do it.

> +
> +static void reg_arg_print(Context *c, YYLTYPE *locp, HexReg *reg)
> +{
> +    char reg_id[5] = { 0 };

This is where the caller zero's out the result ...

> +    reg_compose(c, locp, reg, reg_id);
> +    EMIT(c, "%s", reg_id);
> +}


> +/* Temporary values creation */
> +HexValue gen_tmp(Context *c, YYLTYPE *locp, unsigned bit_width)

You should pass the signedness as an argument rather than assuming it's signed.

> +{
> +    HexValue rvalue;
> +    memset(&rvalue, 0, sizeof(HexValue));
> +    rvalue.type = TEMP;
> +    assert(bit_width == 32 || bit_width == 64);
> +    rvalue.bit_width = bit_width;
> +    rvalue.signedness = SIGNED;
> +    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;
> +}
> +
> +HexValue gen_tmp_value(Context *c,
> +                       YYLTYPE *locp,
> +                       const char *value,
> +                       unsigned bit_width)

You should pass the signedness as an argument rather than assuming it's signed.

> +{
> +    HexValue rvalue;
> +    memset(&rvalue, 0, sizeof(HexValue));
> +    rvalue.type = TEMP;
> +    rvalue.bit_width = bit_width;
> +    rvalue.signedness = SIGNED;
> +    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_const_i", &bit_width, "(", value, ");\n");
> +    c->inst.tmp_count++;
> +    return rvalue;
> +}
> +
> +static HexValue gen_tmp_value_from_imm(Context *c,
> +                                       YYLTYPE *locp,
> +                                       HexValue *value)
> +{
> +    assert(value->type == IMMEDIATE);
> +    HexValue rvalue;
> +    memset(&rvalue, 0, sizeof(HexValue));
> +    rvalue.type = TEMP;
> +    rvalue.bit_width = value->bit_width;
> +    rvalue.signedness = value->signedness;
> +    rvalue.is_dotnew = false;
> +    rvalue.is_manual = false;
> +    rvalue.tmp.index = c->inst.tmp_count;
> +    OUT(c, locp, "TCGv_i", &rvalue.bit_width, " tmp_", &c->inst.tmp_count);
> +    OUT(c, locp, " = tcg_const_i", &rvalue.bit_width,
> +        "((int", &rvalue.bit_width, "_t) (", value, "));\n");

Is the type conversion necessary?  You've copied the bit_width and the 
signedness from the value.

> +    c->inst.tmp_count++;
> +    return rvalue;
> +}
> +
> +HexValue gen_imm_value(Context *c __attribute__((unused)),
> +                       YYLTYPE *locp,
> +                       int value,
> +                       unsigned bit_width)

You should pass the signedness as an argument rather than assuming it's signed.

> +{
> +    (void) locp;
> +    HexValue rvalue;
> +    memset(&rvalue, 0, sizeof(HexValue));
> +    rvalue.type = IMMEDIATE;
> +    rvalue.bit_width = bit_width;
> +    rvalue.signedness = SIGNED;
> +    rvalue.is_dotnew = false;
> +    rvalue.is_manual = false;
> +    rvalue.imm.type = VALUE;
> +    rvalue.imm.value = value;
> +    return rvalue;
> +}


> +void gen_varid_allocate(Context *c,
> +                        YYLTYPE *locp,
> +                        HexValue *varid,
> +                        int width,
> +                        HexSignedness signedness)
> +{
> +    assert_signedness(c, locp, signedness);
> +    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->signedness = other->signedness;

Do you need to check that "other" has the same bit_width and signedness?

> +    } 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,
> +            .signedness = signedness,
> +        };
> +        g_array_append_val(c->inst.allocated, new_var);
> +    }
> +}
> +
> +void gen_operands_extend(Context *c,
> +                         YYLTYPE *locp,
> +                         HexValue *op1,
> +                         HexValue *op2) {
> +    enum OpTypes op_types = (op1->type != IMMEDIATE) << 1
> +                            | (op2->type != IMMEDIATE);
> +
> +    switch (op_types) {
> +    case IMM_IMM:
> +        break;
> +    case IMM_REG:
> +        *op2 = gen_rvalue_extend(c, locp, op2);
> +        break;
> +    case REG_IMM:
> +        *op1 = gen_rvalue_extend(c, locp, op1);
> +        break;
> +    case REG_REG:
> +        *op1 = gen_rvalue_extend(c, locp, op1);
> +        *op2 = gen_rvalue_extend(c, locp, op2);
> +        break;
> +    }
> +}

Why are you only extending the REG operands?  Will you ever need to extend the 
IMM operands (e.g., 32-bit immediate and 64-bit register)?

> +static void gen_andl_op(Context *c, YYLTYPE *locp, unsigned bit_width,
> +                        const char *bit_suffix, HexValue *res,
> +                        enum OpTypes op_types, HexValue *op1, HexValue *op2)
> +{
> +    HexValue zero, tmp1, tmp2;
> +    memset(&zero, 0, sizeof(HexValue));
> +    memset(&tmp1, 0, sizeof(HexValue));
> +    memset(&tmp2, 0, sizeof(HexValue));
> +    switch (op_types) {
> +    case IMM_IMM:
> +        OUT(c, locp, "int", &bit_width, "_t ",
> +            res, " = ", op1, " && ", op2, ";\n");
> +        break;
> +    case IMM_REG:
> +        zero = gen_tmp_value(c, locp, "0", 32);
> +        tmp2 = gen_bin_cmp(c, locp, TCG_COND_NE, op2, &zero);
> +        OUT(c, locp, "tcg_gen_andi_", bit_suffix,
> +            "(", res, ", ", op1, " != 0 , ", &tmp2, ");\n");
> +        gen_rvalue_free(c, locp, &tmp2);
> +        break;
> +    case REG_IMM:
> +        zero = gen_tmp_value(c, locp, "0", 32);
> +        tmp1 = gen_bin_cmp(c, locp, TCG_COND_NE, op1, &zero);
> +        OUT(c, locp, "tcg_gen_andi_", bit_suffix,
> +            "(", res, ", ", &tmp1, ", ", op2, " != 0);\n");
> +        gen_rvalue_free(c, locp, &tmp1);
> +        break;
> +    case REG_REG:
> +        zero = gen_tmp_value(c, locp, "0", 32);
> +        zero.is_manual = true;
> +        tmp1 = gen_bin_cmp(c, locp, TCG_COND_NE, op1, &zero);
> +        tmp2 = gen_bin_cmp(c, locp, TCG_COND_NE, op2, &zero);
> +        OUT(c, locp, "tcg_gen_and_", bit_suffix,
> +            "(", res, ", ", &tmp1, ", ", &tmp2, ");\n");
> +        gen_rvalue_free_manual(c, locp, &zero);
> +        gen_rvalue_free(c, locp, &tmp1);
> +        gen_rvalue_free(c, locp, &tmp2);
> +        break;
> +    }
> +}

This might be OK for everything in the Hexagon semantics files.  However, it 
doesn't match the C semantics of the && operator.
In C, the expression X && Y only evaluates Y after we check that X is true.

> +/* Code generation functions */
> +HexValue gen_bin_op(Context *c,
> +                    YYLTYPE *locp,
> +                    OpType type,
> +                    HexValue *operand1,
> +                    HexValue *operand2)
> +{
> +    /* Replicate operands to avoid side effects */
> +    HexValue op1 = *operand1;
> +    HexValue op2 = *operand2;
> +
> +    /* Enforce variables' size */
> +    if (op1.type == VARID) {
> +        int index = find_variable(c, locp, &op1);
> +        yyassert(c, locp, c->inst.allocated->len > 0,
> +                 "Variable in bin_op must exist!\n");
> +        op1.bit_width = g_array_index(c->inst.allocated, Var, 
> index).bit_width;
> +    }
> +    if (op2.type == VARID) {
> +        int index = find_variable(c, locp, &op2);
> +        yyassert(c, locp, c->inst.allocated->len > 0,
> +                 "Variable in bin_op must exist!\n");
> +        op2.bit_width = g_array_index(c->inst.allocated, Var, 
> index).bit_width;
> +    }

It's not clear what these checks are doing - please add a comment.

> +
> +    enum OpTypes op_types = (op1.type != IMMEDIATE) << 1
> +                            | (op2.type != IMMEDIATE);
> +
> +    bool op_is64bit = op1.bit_width == 64 || op2.bit_width == 64;
> +
> +    /* Shift greater than 32 are 64 bits wide */
> +    if (type == ASL_OP && op2.type == IMMEDIATE &&
> +        op2.imm.type == VALUE && op2.imm.value >= 32)
> +        op_is64bit = true;

Need curly braces per qemu coding conventions

> +
> +    /* Extend to 64-bits, if required */
> +    if (op_is64bit) {
> +        gen_operands_extend(c, locp, &op1, &op2);
> +    }
> +
> +    HexValue res;
> +    memset(&res, 0, sizeof(HexValue));
> +    const char *bit_suffix = op_is64bit ? "i64" : "i32";
> +    int bit_width = (op_is64bit) ? 64 : 32;
> +    if (op_types != IMM_IMM) {
> +        res = gen_tmp(c, locp, bit_width);
> +    } else {
> +        res.type = IMMEDIATE;
> +        res.is_dotnew = false;
> +        res.is_manual = false;
> +        res.imm.type = QEMU_TMP;
> +        res.imm.index = c->inst.qemu_tmp_count;
> +        res.bit_width = bit_width;
> +    }
> +    /* Handle signedness, if both unsigned -> result is unsigned, else signed
> */
> +    assert_signedness(c, locp, op1.signedness);
> +    assert_signedness(c, locp, op2.signedness);
> +    res.signedness = (op1.signedness == UNSIGNED
> +                      && op2.signedness == UNSIGNED) ? UNSIGNED : SIGNED;

This doesn't match C semantics.  You also have to consider the bit_width of the 
original two operands.
    For example, If op1 is unsigned 32 and op2 is signed 64, the result is 
signed

> +HexValue gen_extend_op(Context *c,
> +                       YYLTYPE *locp,
> +                       HexValue *src_width_ptr,
> +                       HexValue *dst_width_ptr,
> +                       HexValue *value_ptr,
> +                       HexSignedness signedness) {
> +    HexValue src_width = *src_width_ptr;
> +    HexValue dst_width = *dst_width_ptr;
> +    HexValue value = *value_ptr;
> +    src_width = gen_rvalue_extend(c, locp, &src_width);
> +    value = gen_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");
> +    assert_signedness(c, locp, signedness);
> +    if (signedness == 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");
> +        gen_rvalue_free(c, locp, &mask);
> +    } else {
> +        OUT(c, locp, "tcg_gen_shl_i64(",
> +            &res, ", ", &value, ", ", &shift, ");\n");
> +        OUT(c, locp, "tcg_gen_sar_i64(",
> +            &res, ", ", &res, ", ", &shift, ");\n");
> +    }

This seems strange that you are doing shifts.  Why not use tcg_gen_extract_* or 
tcg_gen_sextract_*?


> +    OUT(c, locp, "tcg_gen_movcond_i64(TCG_COND_EQ, ", &res, ", ");
> +    OUT(c, locp, &src_width, ", ", &zero, ", ", &zero, ", ", &res, ");\n");
> +
> +    gen_rvalue_free(c, locp, &src_width);
> +    gen_rvalue_free(c, locp, &dst_width);
> +    gen_rvalue_free(c, locp, &value);
> +    gen_rvalue_free(c, locp, &shift);
> +    gen_rvalue_free(c, locp, &zero);
> +
> +    res.signedness = signedness;
> +    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 = gen_rvalue_extend(c, locp, value);
> +    HexValue begin_m = gen_rvalue_extend(c, locp, begin);
> +    HexValue width_orig = *width;
> +    width_orig.is_manual = true;
> +    HexValue width_m = gen_rvalue_extend(c, locp, &width_orig);
> +    width_m = rvalue_materialize(c, locp, &width_m);
> +
> +    HexValue mask = gen_tmp_value(c, locp, "0xffffffffffffffffUL", 64);
> +    mask.signedness = UNSIGNED;
> +    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);

Why not use tcg_gen_deposit_*?

> +
> +    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);
> +
> +    if (dest->bit_width != res.bit_width) {
> +        res = gen_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");

Not sure what this is for?

> +
> +    gen_rvalue_free(c, locp, &zero);
> +    gen_rvalue_free(c, locp, width);
> +    gen_rvalue_free(c, locp, &res);
> +}

> +static HexValue gen_convround_n_a(Context *c,
> +                                  YYLTYPE *locp,
> +                                  HexValue *a,
> +                                  HexValue *n)
> +{
> +    (void) n;
> +    HexValue res = gen_tmp(c, locp, 64);
> +    OUT(c, locp, "tcg_gen_ext_i32_i64(", &res, ", ", a, ");\n");
> +    return res;
> +}

This looks like a simple extend, why is it a separate function?

> +
> +static HexValue gen_convround_n_b(Context *c,
> +                                  YYLTYPE *locp,
> +                                  HexValue *a,
> +                                  HexValue *n)
> +{
> +    HexValue res = gen_tmp(c, locp, 64);
> +    OUT(c, locp, "tcg_gen_ext_i32_i64(", &res, ", ", a, ");\n");
> +
> +    HexValue one = gen_tmp_value(c, locp, "1", 32);
> +    HexValue tmp = gen_tmp(c, locp, 32);
> +    HexValue tmp_64 = gen_tmp(c, locp, 64);
> +
> +    OUT(c, locp, "tcg_gen_shl_i32(", &tmp);
> +    OUT(c, locp, ", ", &one, ", ", n, ");\n");
> +    OUT(c, locp, "tcg_gen_and_i32(", &tmp);
> +    OUT(c, locp, ", ", &tmp, ", ", a, ");\n");
> +    OUT(c, locp, "tcg_gen_shri_i32(", &tmp);
> +    OUT(c, locp, ", ", &tmp, ", 1);\n");
> +    OUT(c, locp, "tcg_gen_ext_i32_i64(", &tmp_64, ", ", &tmp, ");\n");
> +    OUT(c, locp, "tcg_gen_add_i64(", &res);
> +    OUT(c, locp, ", ", &res, ", ", &tmp_64, ");\n");

Why not use tcg_gen_sextract_*?

> +
> +    gen_rvalue_free(c, locp, &one);
> +    gen_rvalue_free(c, locp, &tmp);
> +    gen_rvalue_free(c, locp, &tmp_64);
> +
> +    return res;
> +}
> +
> +static HexValue gen_convround_n_c(Context *c,
> +                                  YYLTYPE *locp,
> +                                  HexValue *a,
> +                                  HexValue *n)
> +{
> +    HexValue res = gen_tmp(c, locp, 64);
> +    OUT(c, locp, "tcg_gen_ext_i32_i64(", &res, ", ", a, ");\n");
> +
> +    HexValue one = gen_tmp_value(c, locp, "1", 32);
> +    HexValue tmp = gen_tmp(c, locp, 32);
> +    HexValue tmp_64 = gen_tmp(c, locp, 64);
> +
> +    OUT(c, locp, "tcg_gen_subi_i32(", &tmp);
> +    OUT(c, locp, ", ", n, ", 1);\n");
> +    OUT(c, locp, "tcg_gen_shl_i32(", &tmp);
> +    OUT(c, locp, ", ", &one, ", ", &tmp, ");\n");
> +    OUT(c, locp, "tcg_gen_ext_i32_i64(", &tmp_64, ", ", &tmp, ");\n");
> +    OUT(c, locp, "tcg_gen_add_i64(", &res);
> +    OUT(c, locp, ", ", &res, ", ", &tmp_64, ");\n");
> +
> +    gen_rvalue_free(c, locp, &one);
> +    gen_rvalue_free(c, locp, &tmp);
> +    gen_rvalue_free(c, locp, &tmp_64);
> +
> +    return res;
> +}
> +
> +HexValue gen_convround_n(Context *c,
> +                         YYLTYPE *locp,
> +                         HexValue *source_ptr,
> +                         HexValue *bit_pos_ptr)
> +{
> +    /* If input is 64 bit cast it to 32 */
> +    HexValue source = gen_cast_op(c, locp, source_ptr, 32);
> +    HexValue bit_pos = gen_cast_op(c, locp, bit_pos_ptr, 32);
> +
> +    source = rvalue_materialize(c, locp, &source);
> +    bit_pos = rvalue_materialize(c, locp, &bit_pos);
> +
> +    HexValue r1 = gen_convround_n_a(c, locp, &source, &bit_pos);
> +    HexValue r2 = gen_convround_n_b(c, locp, &source, &bit_pos);
> +    HexValue r3 = gen_convround_n_c(c, locp, &source, &bit_pos);
> +
> +    HexValue l_32 = gen_tmp_value(c, locp, "1", 32);
> +
> +    HexValue cond = gen_tmp(c, locp, 32);
> +    HexValue cond_64 = gen_tmp(c, locp, 64);
> +    HexValue mask = gen_tmp(c, locp, 32);
> +    HexValue n_64 = gen_tmp(c, locp, 64);
> +    HexValue res = gen_tmp(c, locp, 64);
> +    HexValue zero = gen_tmp_value(c, locp, "0", 64);
> +
> +    OUT(c, locp, "tcg_gen_sub_i32(", &mask);
> +    OUT(c, locp, ", ", &bit_pos, ", ", &l_32, ");\n");
> +    OUT(c, locp, "tcg_gen_shl_i32(", &mask);
> +    OUT(c, locp, ", ", &l_32, ", ", &mask, ");\n");
> +    OUT(c, locp, "tcg_gen_sub_i32(", &mask);
> +    OUT(c, locp, ", ", &mask, ", ", &l_32, ");\n");
> +    OUT(c, locp, "tcg_gen_and_i32(", &cond);
> +    OUT(c, locp, ", ", &source, ", ", &mask, ");\n");
> +    OUT(c, locp, "tcg_gen_extu_i32_i64(", &cond_64, ", ", &cond, ");\n");
> +    OUT(c, locp, "tcg_gen_ext_i32_i64(", &n_64, ", ", &bit_pos, ");\n");

Some comments here would be helpful ...

> +
> +    OUT(c, locp, "tcg_gen_movcond_i64");
> +    OUT(c, locp, "(TCG_COND_EQ, ", &res, ", ", &cond_64, ", ", &zero);
> +    OUT(c, locp, ", ", &r2, ", ", &r3, ");\n");
> +
> +    OUT(c, locp, "tcg_gen_movcond_i64");
> +    OUT(c, locp, "(TCG_COND_EQ, ", &res, ", ", &n_64, ", ", &zero);
> +    OUT(c, locp, ", ", &r1, ", ", &res, ");\n");
> +
> +    OUT(c, locp, "tcg_gen_shr_i64(", &res);
> +    OUT(c, locp, ", ", &res, ", ", &n_64, ");\n");
> +
> +    gen_rvalue_free(c, locp, &source);
> +    gen_rvalue_free(c, locp, &bit_pos);
> +
> +    gen_rvalue_free(c, locp, &r1);
> +    gen_rvalue_free(c, locp, &r2);
> +    gen_rvalue_free(c, locp, &r3);
> +
> +    gen_rvalue_free(c, locp, &cond);
> +    gen_rvalue_free(c, locp, &cond_64);
> +    gen_rvalue_free(c, locp, &l_32);
> +    gen_rvalue_free(c, locp, &mask);
> +    gen_rvalue_free(c, locp, &n_64);
> +    gen_rvalue_free(c, locp, &zero);
> +
> +    res = gen_rvalue_truncate(c, locp, &res);
> +    return res;
> +}
> +
> +HexValue gen_round(Context *c,
> +                   YYLTYPE *locp,
> +                   HexValue *source,
> +                   HexValue *position) {
> +    yyassert(c, locp, source->bit_width <= 32,
> +             "fRNDN not implemented for bit widths > 32!");
> +
> +    HexValue src = *source;
> +    HexValue pos = *position;
> +
> +    HexValue src_width = gen_imm_value(c, locp, src.bit_width, 32);
> +    HexValue dst_width = gen_imm_value(c, locp, 64, 32);
> +    HexValue a = gen_extend_op(c, locp, &src_width, &dst_width, &src,
> SIGNED);

Are you sure extending is the right thing to do here?



> +HexValue gen_fbrev_4(Context *c, YYLTYPE *locp, HexValue *source)
> +{
> +    HexValue source_m = *source;
> +
> +    HexValue res = gen_tmp(c, locp, 32);
> +    HexValue tmp1 = gen_tmp(c, locp, 32);
> +    HexValue tmp2 = gen_tmp(c, locp, 32);
> +
> +    source_m = rvalue_materialize(c, locp, &source_m);
> +    source_m = gen_rvalue_truncate(c, locp, &source_m);
> +
> +    OUT(c, locp, "tcg_gen_mov_tl(", &res, ", ", &source_m, ");\n");
> +    OUT(c, locp, "tcg_gen_andi_tl(", &tmp1, ", ", &res, ", 0xaaaaaaaa);\n");
> +    OUT(c, locp, "tcg_gen_shri_tl(", &tmp1, ", ", &tmp1, ", 1);\n");
> +    OUT(c, locp, "tcg_gen_andi_tl(", &tmp2, ", ", &res, ", 0x55555555);\n");
> +    OUT(c, locp, "tcg_gen_shli_tl(", &tmp2, ", ", &tmp2, ", 1);\n");
> +    OUT(c, locp, "tcg_gen_or_tl(", &res, ", ", &tmp1, ", ", &tmp2, ");\n");
> +    OUT(c, locp, "tcg_gen_andi_tl(", &tmp1, ", ", &res, ", 0xcccccccc);\n");
> +    OUT(c, locp, "tcg_gen_shri_tl(", &tmp1, ", ", &tmp1, ", 2);\n");
> +    OUT(c, locp, "tcg_gen_andi_tl(", &tmp2, ", ", &res, ", 0x33333333);\n");
> +    OUT(c, locp, "tcg_gen_shli_tl(", &tmp2, ", ", &tmp2, ", 2);\n");
> +    OUT(c, locp, "tcg_gen_or_tl(", &res, ", ", &tmp1, ", ", &tmp2, ");\n");
> +    OUT(c, locp, "tcg_gen_andi_tl(", &tmp1, ", ", &res, ", 0xf0f0f0f0);\n");
> +    OUT(c, locp, "tcg_gen_shri_tl(", &tmp1, ", ", &tmp1, ", 4);\n");
> +    OUT(c, locp, "tcg_gen_andi_tl(", &tmp2, ", ", &res, ", 0x0f0f0f0f);\n");
> +    OUT(c, locp, "tcg_gen_shli_tl(", &tmp2, ", ", &tmp2, ", 4);\n");
> +    OUT(c, locp, "tcg_gen_or_tl(", &res, ", ", &tmp1, ", ", &tmp2, ");\n");
> +    OUT(c, locp, "tcg_gen_bswap32_tl(", &res, ", ", &res, ",
> TCG_BSWAP_IZ);\n");

This would be better as a helper that just does revbit32 - or maybe just skip 
any instructions that reference this.

> +HexValue gen_fbrev_8(Context *c, YYLTYPE *locp, HexValue *source)

Ditto


> +HexValue gen_deinterleave(Context *c, YYLTYPE *locp, HexValue *mixed)

Ditto

> +
> +HexValue gen_interleave(Context *c,
> +                        YYLTYPE *locp,
> +                        HexValue *odd,
> +                        HexValue *even)

Ditto


> +void gen_pre_assign(Context *c, YYLTYPE *locp, HexValue *lp, HexValue
> *rp)

Change to gen_pred_assign

> +void gen_load(Context *c, YYLTYPE *locp, HexValue *size,
> +              HexSignedness signedness, HexValue *ea, HexValue *dst)
> +{
> +    /* Memop width is specified in the load macro */
> +    int bit_width = (size->imm.value > 4) ? 64 : 32;
> +    assert_signedness(c, locp, signedness);
> +    const char *sign_suffix = (size->imm.value > 4)
> +                              ? ""
> +                              : ((signedness == 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.
> */
> +        gen_varid_allocate(c, locp, dst, bit_width, signedness);
> +    }
> +    snprintf(size_suffix, 4, "%" PRIu64, size->imm.value * 8);
> +    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;
> +    OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_store_s1) {\n");
> +    OUT(c, locp, "process_store(ctx, pkt, 1);\n");
> +    OUT(c, locp, "}\n");
> +    OUT(c, locp, "tcg_gen_qemu_ld", size_suffix, sign_suffix);
> +    OUT(c, locp, "(", dst, ", ", ea, ", 0);\n");

Instead of 0, pass ctx->mem_idx

> +    /* If the var in EA was truncated it is now a tmp HexValue, so free it. 
> */
> +    gen_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 = gen_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");

Taking the neg only works if val is 0 or 1.  Today, this is always invoked with 
0 or 1, but this implementation isn't future-proof.  Do an andi with 1 first, 
then it will be correct for possible inputs.

> +    OUT(c, locp, "tcg_gen_deposit_i32(", dst, ", ", dst, ", ", &tmp, ", ");
> +    OUT(c, locp, lo, ", ", &len, ");\n");
> +
> +    gen_rvalue_free(c, locp, &tmp);
> +    gen_rvalue_free(c, locp, hi);
> +    gen_rvalue_free(c, locp, lo);
> +    gen_rvalue_free(c, locp, val);
> +}


> +HexValue gen_rvalue_pre(Context *c, YYLTYPE *locp, HexValue *pre)

Call this gen_rvalue_pred and name the argument pred

> +HexValue gen_rvalue_var(Context *c, YYLTYPE *locp, HexValue *var)
> +{
> +    /* Assign correct bit width and signedness */
> +    bool found = false;
> +    for (unsigned i = 0; i < c->inst.allocated->len; i++) {
> +        Var *other = &g_array_index(c->inst.allocated, Var, i);
> +        if (g_string_equal(var->var.name, other->name)) {

Use find_variable function here?

> +            found = true;
> +            other->name = var->var.name;
> +            var->bit_width = other->bit_width;
> +            var->signedness = other->signedness;
> +            break;
> +        }
> +    }
> +    yyassert(c, locp, found, "Undefined symbol!\n");
> +    return *var;
> +}


> +HexValue gen_rvalue_not(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;
> +    memset(&res, 0, sizeof(HexValue));
> +    res.signedness = v->signedness;
> +    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, " = ~", v, ";\n");

I see a lot of these casts to "int", &bit_width, "_t".  Shouldn't we also be 
checking the signedness and decide if it should be uint... instead?

> +        c->inst.qemu_tmp_count++;
> +    } else {
> +        res = gen_tmp(c, locp, bit_width);
> +        OUT(c, locp, "tcg_gen_not_", bit_suffix, "(", &res,
> +            ", ", v, ");\n");
> +        gen_rvalue_free(c, locp, v);
> +    }
> +    return res;
> +}


> +HexValue gen_rvalue_abs(Context *c, YYLTYPE *locp, HexValue *v)
> +{
> +    int bit_width = (v->bit_width == 64) ? 64 : 32;
> +    HexValue res;
> +    memset(&res, 0, sizeof(HexValue));
> +    res.signedness = v->signedness;
> +    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);
> +        OUT(c, locp, "tcg_gen_abs_i", &bit_width, "(", &res, ", ", v, 
> ");\n");
> +        gen_rvalue_free(c, locp, v);
> +    }
> +    return res;
> +}
> +
> +HexValue gen_rvalue_neg(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;
> +    memset(&res, 0, sizeof(HexValue));
> +    res.signedness = v->signedness;
> +    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, " = -", v, ";\n");
> +        c->inst.qemu_tmp_count++;
> +    } else {
> +        res = gen_tmp(c, locp, bit_width);
> +        OUT(c, locp, "tcg_gen_neg_", bit_suffix, "(", &res, ", ", v, ");\n");
> +        gen_rvalue_free(c, locp, v);
> +    }
> +    return res;
> +}

There's too much copy&paste across these.  I'd suggest a helper function that 
takes the C code (e.g., "-") and the TCG function (e.g., tcg_gen_neg_) as 
arguments.



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


> +       | CONSTEXT
> +         {
> +             HexValue rvalue;
> +             memset(&rvalue, 0, sizeof(HexValue));
> +             rvalue.type = IMMEDIATE;
> +             rvalue.imm.type = IMM_CONSTEXT;
> +             rvalue.signedness = UNSIGNED;
> +             rvalue.is_dotnew = false;
> +             rvalue.is_manual = false;
> +             $$ = rvalue;
> +         }

Aren't the constant extenders already taken care of during decoding?


> +       | CAST rvalue
> +         {
> +             @1.last_column = @2.last_column;
> +             /* Assign target signedness */
> +             $2.signedness = $1.signedness;
> +             $$ = gen_cast_op(c, &@1, &$2, $1.bit_width);
> +             $$.signedness = $1.signedness;

Pass $1.signedness to gen_cast_op rather than setting it here

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

You're assuming $1 is a bitfield.  How do you assure it's not a normal array?