Re: [Qemu-devel] [PATCH v2 11/14] tcg: Implement indirect memory registers

[Richard Henderson]

On 12/31/2015 03:54 AM, Aurelien Jarno wrote:
> On 2015-12-17 12:00, Richard Henderson wrote:
>> That is, global_mem registers whose base is another global_mem
>> register, rather than a fixed register.
>>
>> Signed-off-by: Richard Henderson <address@hidden>
>> ---
>>  tcg/tcg.c | 95 
>> ++++++++++++++++++++++++++++++++++++++++++++-------------------
>>  tcg/tcg.h |  2 ++
>>  2 files changed, 68 insertions(+), 29 deletions(-)
>>
>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>> index c51e0ec..7150a3f 100644
>> --- a/tcg/tcg.c
>> +++ b/tcg/tcg.c
>> @@ -509,17 +509,23 @@ int tcg_global_mem_new_internal(TCGType type, TCGv_ptr 
>> base,
>>      TCGContext *s = &tcg_ctx;
>>      TCGTemp *base_ts = &s->temps[GET_TCGV_PTR(base)];
>>      TCGTemp *ts = tcg_global_alloc(s);
>> -    int bigendian = 0;
>> +    int indirect_reg = 0, bigendian = 0;
>>  #ifdef HOST_WORDS_BIGENDIAN
>>      bigendian = 1;
>>  #endif
>>  
>> +    if (!base_ts->fixed_reg) {
>> +        indirect_reg = 1;
>> +        base_ts->indirect_base = 1;
>> +    }
>> +
>>      if (TCG_TARGET_REG_BITS == 32 && type == TCG_TYPE_I64) {
>>          TCGTemp *ts2 = tcg_global_alloc(s);
>>          char buf[64];
>>  
>>          ts->base_type = TCG_TYPE_I64;
>>          ts->type = TCG_TYPE_I32;
>> +        ts->indirect_reg = indirect_reg;
> 
> Do we really need to add this new bit, while we can simply test
> ts->mem_base->fixed_reg? This means one more derefence, but anyway it
> has to be done later when calling temp_load?

I thought it cleaner to have the bit.  We're using 7 of 32 (or 39 of 64) from
the word, so we're not really short of bit space.

>> +    /* ??? Liveness does not yet incorporate indirect bases.  */
>> +    if (!ts->indirect_base) {
>> +        /* The liveness analysis already ensures that globals are back
>> +           in memory. Keep an assert for safety. */
>> +        tcg_debug_assert(ts->val_type == TEMP_VAL_MEM || ts->fixed_reg);
>> +        return;
>> +    }
> 
> This basically disables the assert. What does it mean in practice? Can
> it generates bad code?

Since we have no liveness info for the indirect base, we don't know exactly
where it begins and ends life.  That could be fixed with additional effort,
but it didn't seem to be required during this initial implementation.

Since we don't have liveness info, I simply want to follow the path we would
take when liveness info isn't computed.  We do not generate bad code.


r~