Re: [PATCH 05/15] arc: TCG instruction generator and hand-definitions

[Cupertino Miranda]

Thanks for your quick reply.

On 1/15/21 8:17 PM, Richard Henderson wrote:
> On 1/15/21 7:11 AM, Cupertino Miranda wrote:
>>> On 11/11/20 10:17 AM, cupertinomiranda@gmail.com wrote:
>>>> +/*
>>>> + * The macro to add boiler plate code for conditional execution.
>>>> + * It will add tcg_gen codes only if there is a condition to
>>>> + * be checked (ctx->insn.cc != 0). This macro assumes that there
>>>> + * is a "ctx" variable of type "DisasCtxt *" in context. Remember
>>>> + * to pair it with CC_EPILOGUE macro.
>>>> + */
>>>> +#define CC_PROLOGUE                                   \
>>>> +  TCGv cc = tcg_temp_local_new();                     \
>>>> +  TCGLabel *done = gen_new_label();                   \
>>>> +  do {                                                \
>>>> +    if (ctx->insn.cc) {                               \
>>>> +        arc_gen_verifyCCFlag(ctx, cc);                \
>>>> +        tcg_gen_brcondi_tl(TCG_COND_NE, cc, 1, done); \
>>>> +    }                                                 \
>>>> +  } while (0)
>>>> +
>>>> +/*
>>>> + * The finishing counter part of CC_PROLUGE. This is supposed
>>>> + * to be put at the end of the function using it.
>>>> + */
>>>> +#define CC_EPILOGUE          \
>>>> +    if (ctx->insn.cc) {      \
>>>> +        gen_set_label(done); \
>>>> +    }                        \
>>>> +    tcg_temp_free(cc)
>>>
>>> Why would this need to be boiler-plate?  Why would this not simply exist in
>>> exactly one location?
>>>
>>> You don't need a tcg_temp_local_new, because the cc value is not used past 
>>> the
>>> branch.  You should free the temp immediately after the branch.
>>>
>>
>> I wonder if what you thought was to move those macros to functions and
>> call it when CC_PROLOGUE and CC_EPILOGUE are used.
>> I think the macros were choosen due to the sharing of the 'cc' and
>> 'done' variables in both CC_PROLOGUE AND CC_EPILOGUE.
> 
> I meant that the checking of ctx->insn.cc could be done at a higher level, so
> that this code existed in exactly one place, not scattered between all of the
> different instructions.
> 
> But if that isn't possible for some reason, you can certainly put "done" into
> the DisasContext so that you can have to functions cc_prologue(ctx) and
> cc_epilogue(ctx).
> 
> 
>>>> +void gen_goto_tb(DisasContext *ctx, int n, TCGv dest)
>>>> +{
>>>> +    tcg_gen_mov_tl(cpu_pc, dest);
>>>> +    tcg_gen_andi_tl(cpu_pcl, dest, 0xfffffffc);
>>>> +    if (ctx->base.singlestep_enabled) {
>>>> +        gen_helper_debug(cpu_env);
>>>> +    }
>>>> +    tcg_gen_exit_tb(NULL, 0);
>>>
>>> Missing else.  This is dead code for single-step.
>> Goes a little above my knowledge of QEMU internals to be honest.
>> Do you have a recommendation what we should be doing here ?
> 
> Both of these actions end the TB, so:
> 
>    if (ctx->base.singlestep_enabled) {
>      gen_helper_debug(cpu_env);
>    } else {
>      tcg_gen_exit_tb(NULL, 0);
>    }
> 
Clear !!! :-)

>>> You could put all of these into a const static table.
>> What do you mean, can we make the effect of tcg_global_mem_new_i32 as
>> constant ?
> 
> No, I mean all of the data that is passed to tcg_global_mem_new.  See for
> instance target/sparc/translate.c, sparc_tcg_init().
Clear.
> 
> 
>>>> +static void init_constants(void)
>>>> +{
>>>> +#define SEMANTIC_FUNCTION(...)
>>>> +#define MAPPING(...)
>>>> +#define CONSTANT(NAME, MNEMONIC, OP_NUM, VALUE) \
>>>> +  add_constant_operand(MAP_##MNEMONIC##_##NAME, OP_NUM, VALUE);
>>>> +#include "target/arc/semfunc_mapping.def"
>>>> +#include "target/arc/extra_mapping.def"
>>>> +#undef MAPPING
>>>> +#undef CONSTANT
>>>> +#undef SEMANTIC_FUNCTION
>>>> +}
>>>
>>> Ew.  Yet another thing that can be done at build time.
>> As far as I remember it, there was no way I could generate this table
>> using the C pre-processor. Do you suggest to make this table using an
>> external tool ?
> 
> I assumed that you could just as easily generate a table using the c
> preprocessor as this function.  I guess I'd like to know more about why you
> can't...

To be fair, it would be possible but not so economical.
This would actually be a 2 dimensional table of size ((NAME * MNEMONIC) 
x (3)). 3 is the assumed maximum operand size.

In order to minimize wasted space the second dimension was implemented 
as a linked list.
Considering also this the entries in the table would also need to be of 
type struct, as we needed to mark somehow the entries that did not 
define a CONSTANT.

Please notice there are only 16 entries of this CONSTANT macro, making 
this initialization negligible.

> 
>>>> +            int32_t limm = operand.value;
>>>> +            if (operand.type & ARC_OPERAND_LIMM) {
>>>> +                limm = ctx->insn.limm;
>>>> +                tcg_gen_movi_tl(cpu_limm, limm);
>>>> +                ret = cpu_r[62];
>>>> +            } else {
>>>> +                ret = tcg_const_local_i32(limm);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +  return ret;
>>>
>>> Why are you using locals for everything?  Is it because you have no proper
>>> control over your use of branching?
>>
>> Initially we though locals the good way to define temporaries. :-(
>> What should be the best ? We will need to change a lot of code for this.
> 
> TCG is a poor optimizer.  If you can at all avoid branches, while implementing
> a single instruction, do so.  Because this means that you can use
> tcg_temp_new_i32 (et al) which are "normal" temps, and are not spilled to the
> stack at the end of the BB.
> 
> This does not necessarily apply to conditional execution (cc_prologue, et al)
> because you can think of those as "outside" of the instruction, merely 
> wrapping
> them.  The actual live temps will be "inside" and not live past the done 
> label.

Maybe I will need to make my tcg code generator aware of this in order 
to properly create temps.

>>>> +/* Return from exception. */
>>>> +static void gen_rtie(DisasContext *ctx)
>>>> +{
>>>> +    tcg_gen_movi_tl(cpu_pc, ctx->cpc);
>>>> +    gen_helper_rtie(cpu_env);
>>>> +    tcg_gen_mov_tl(cpu_pc, cpu_pcl);
>>>> +    gen_goto_tb(ctx, 1, cpu_pc);
>>>> +}
>>>
>>> You must return to the main loop here, not goto_tb.  You must return to the
>>> main loop every time your interrupt mask changes, so that pending interrupts
>>> may be accepted.
>>>
>> "gen_goto_tb" calls in the end "tcg_gen_exit_tb(NULL, 0)", is it not the
>> same ?
> 
> No.  Because gen_goto_tb uses tcg_gen_goto_tb, which ends the TB right there.
> Another instance of the "else" bug above.
> 
>> We need to investigate this implementation further. A quick change to
>> gen_rtie broke linux booting.
>> Can you recomend some target that implements the loop exit on rtie as
>> you suggest ?
> 
> target/riscv/ -- see trans_mret() and exit_tb().
Clear.
> 
> 
> r~
>