Re: [Qemu-devel] [PATCH 07/35] tcg: Optionally sign-extend 32-bit arguments for 64-bit host.

[Richard Henderson]

On 06/10/2010 03:22 AM, Aurelien Jarno wrote:
>> -                  0, GET_TCGV_I32(ret), 2, args);
>> +                  (is_signed ? 0x2a : 0x00), GET_TCGV_I32(ret), 2, args);
> 
> Wouldn't it be better to actually pass the whole flag to
> tcg_gen_helper32(), so that we can in the future also support mixed
> signedness in arguments? Also doing it here looks like a bit like a
> magic constant.

I've fixed this.

>> +#if defined(TCG_TARGET_EXTEND_ARGS) && TCG_TARGET_REG_BITS == 64
>> +    for (i = 0; i < nargs; ++i) {
>> +        int is_64bit = sizemask & (1 << (i+1)*2);
>> +        int is_signed = sizemask & (2 << (i+1)*2);
>> +        if (!is_64bit) {
>> +            TCGv_i64 temp = tcg_temp_new_i64();
>> +            TCGv_i64 orig = MAKE_TCGV_I64(args[i]);
>> +            if (is_signed) {
>> +                tcg_gen_ext32s_i64(temp, orig);
>> +            } else {
>> +                tcg_gen_ext32u_i64(temp, orig);
>> +            }
>> +            args[i] = GET_TCGV_I64(temp);
>> +        }
>> +    }
>> +#endif /* TCG_TARGET_EXTEND_ARGS */
>> +
> 
> This part allocates a lot of temp variables, that will probably generate
> a lot of register spills during the code generation.
> 
> As we do that for all arguments anyway, wouldn't it be possible to do
> the extension in place? The value in the register is changed, but that
> should not have any effect as it is ignored anyway in other
> instructions.

It is *not* possible to do the extension in-place.  At least not without
changing the format of the INDEX_op_call opcode.

With the extension done during opcode generation, like this, ORIG has been
marked TCG_TYPE_I32 and TEMP gets marked TCG_TYPE_I64.  This matters when
it comes time to copy the arguments into place.  If we somehow extended 
ORIG in-place, we'd still use the wrong instruction to copy the value into
the argument list -- either tcg_out_mov or tcg_out_st would get the wrong
TYPE argument.

If we try to do the extension later, e.g. while copying the value into the
argument list, we'd need to have SIZEMASK available.  To do that, we'd need
to save SIZEMASK into INDEX_op_call's argument list somehow.  That, I think,
is a more invasive change.


r~