Re: [PATCH v2 08/10] target/hexagon: import parser for idef-parser

[Paolo Montesel via <qemu-devel@nongnu.org>]

Thanks for the feedback, it helped us improve the implementation quite a bit.

> > +| rvalue QMARK rvalue COLON rvalue
> > +{
> > +    @1.last_column = @5.last_column;
> > +    bool is_64bit = ($3.bit_width == 64) || ($5.bit_width == 64);
> > +    int bit_width = (is_64bit) ? 64 : 32;
> > +    if (is_64bit) {
> > +        $1 = rvalue_extend(c, &@1, &$1);
> > +        $3 = rvalue_extend(c, &@1, &$3);
> > +        $5 = rvalue_extend(c, &@1, &$5);
> > +    } else {
> > +        $1 = rvalue_truncate(c, &@1, &$1);
> > +    }
> > +    $1 = rvalue_materialize(c, &@1, &$1);
> > +    $3 = rvalue_materialize(c, &@1, &$3);
> > +    $5 = rvalue_materialize(c, &@1, &$5);
> > +    HexValue res = gen_local_tmp(c, &@1, bit_width);
> > +    HexValue zero = gen_tmp_value(c, &@1, "0", bit_width);
> > +    OUT(c, &@1, "tcg_gen_movcond_i", &bit_width);
> > +    OUT(c, &@1, "(TCG_COND_NE, ", &res, ", ", &$1, ", ", &zero);
>
> It would be better if you parsed conditions differently.
> Retain the two arguments and the condition, so that you can fold that into the
> movcond directly.
>
> E.g. instead of
>
>     tcg_gen_setcond_i32(cond, t, x, y)
>     tcg_gen_movcond_i32(TCG_COND_NE, dest, t, zero, src1, src2);
>
> you'd be able to do
>
>     tcg_gen_movcond_i32(cond, dest, x, y, src1, src2);
>
> This would be trivial with a non-terminal "cond", used here and with IF.  You'd
> include cond as an alternative of rvalue, which would perform the reduction to
> boolean with setcond.

This would save us from emitting some tcg ops but would increase the
complexity of the parser, which doesn't seem worth it imho.

> > +    case VALUE:
> > +        EMIT(c, "((int64_t)%" PRIu64 "ULL)", (int64_t)imm->value);
>
> Why are you using ull then casting to signed?  Just use ll.

We have a case in which we would print `-9223372036854775808LL` (64
bit integer with sign bit set) and gcc would complain with `warning:
integer constant is so large that it is unsigned`.
That's the reason for using ULL and then casting.
I'm open to other solutions.

> > +    switch (op_types) {
> > +    case IMM_IMM:
> > +    {
> > +        OUT(c, locp, "tcg_gen_movi_", bit_suffix,
> > +            "(", &res, ", ", &op1, " == ", &op2, ");\n");
> > +        break;
> > +    }
>
> Drop useless braces like this.
>
> Do you really see any IMM_IMM operations?  There are some typos in this
> section, so certainly all operators are not represented.  It might be worth
> folding all of these inside the parser, and not deferring to the C compiler, so
> that you can be certain of having a real value for any IMMEDIATE.  Which will
> help when it comes to shift below.

Maybe not for all bin ops, but we do see IMM_IMM.
I think we can't always fold IMMEDIATES, because they include "normal"
C variables (e.g.: `int32_t uiV` in function arguments) that depend on
instruction bytes at runtime.
That's true also for the IMM_IMM case.

> I'm thinking that this code could really benefit from tcg_constant_{i32,i64}.
> It produces a hashed temp that need not be freed, and it's what many of the
> tcg_gen_fooi functions use in the backend.

We can technically convert all the IMMs to tcg_constant before using
them, but since we can't constant fold in the parser that would
probably decrease performance quite a bit.

> > +static void gen_div_op(Context *c, YYLTYPE *locp, HexValue *res,
> > +                       enum OpTypes op_types, HexValue *op1, HexValue *op2)
> > +{
> > +    switch (op_types) {
> > +    case IMM_IMM:
> > +        OUT(c, locp, "int64_t ", res, " = ", op1, " / ", op2, ";\n");
> > +        break;
> > +    case IMM_REG:
> > +    case REG_IMM:
> > +    case REG_REG:
> > +        OUT(c, locp, res, " = gen_helper_divu("
> > +            "cpu_env, ", op1, ", ", op2, ");\n");
>
> Are we trusting that div-by-zero has already been tested for?

Turns out div is not even used by the instructions we currently
support (: so we can just delete this

~Paolo