Re: [Qemu-devel] [RFC] TCG new op: setcond

[Laurent Desnogues]

On Tue, Nov 4, 2008 at 2:16 PM, Paul Brook <address@hidden> wrote:
>> this patch implements a new TCG op, setcond, that sets a temp
>> to 1 if the condition is true, else to 0.  The benefit is the potential
>> removal of brcond instructions, and helpers size reduction which
>> can lead to using TCG instead of helpers.
>
>>   - a variant that sets -1 instead of 1 for masking
>
> I'm worried about this. If we're not careful we'll end up with an explosion of
> different patterns, many of which aren't optimal of different hosts.

I am worried too.  I added that because you mentioned you would
prefer -1 over 1.  I am still unsure which one of these variants is
the most useful and/or host runtime critical (one can be derived
from the other by generating one extra TCG op).

>>   - 64 bit setcond's
>
> You should do this sooner rather than later, and on a 32-bit host.

OK.

>> +    /* clear ret since setcc only sets the lower 8 bits */
>> +    tcg_out_modrm(s, 0x01 | (ARITH_XOR << 3) | rexw, ret, ret);
>
> This is broken. Inputs and outputs may overlap.

Oh right.  I will use some bit extending instruction.

>> +    // TODO this should use tcg_out_modrm
>> +    //      however currently tcg_out_modrm outputs an extra byte for
>> [abcd]l +    //tcg_out_modrm(s, (0x90 + tcg_cond_to_jcc[cond]) | P_EXT |
>> P_REXB, ret, 0)
>
> This is the wrong way to fix this. If you really care about the extra code
> byte (which is harmless) you should fix tcg_out_modrm.

I asked about that directly to Fabrice yesterday.  I am waiting for
his answer.  I certainly am not very good at x86 and its baroque
encoding :)

> Also, please use C comments, not c++ style //.

These were done so that it is explicit they are here temporarily for
reviewers.  They will disappear in the final patch.

Thanks for the comments.


Laurent