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, cupertinomira...@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~
>
