[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 08/10] target/hexagon: import parser for idef-parser
From: |
Paolo Montesel |
Subject: |
Re: [PATCH v2 08/10] target/hexagon: import parser for idef-parser |
Date: |
Tue, 23 Mar 2021 17:52:08 +0100 |
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