Re: [Qemu-devel] tcg conditional set/move, round 3

[Aurelien Jarno]

On Fri, Dec 18, 2009 at 01:38:24PM -0800, Richard Henderson wrote:
> On 12/18/2009 03:37 AM, Laurent Desnogues wrote:
>>>   tcg: Generic support for conditional set and conditional move.
>>
>> Needs cosmetics changes.
>
> Fixed, attachment 1.
>
>>>   tcg-x86_64: Implement setcond and movcond.
>>
>> Some cosmetics and comments, but overall good.
>
> Fixed, attachment 2.
>
>>>   tcg-i386: Implement small forward branches.
>>
>> I think this contains a bug.
>
> Fixed, attachment 3.  I've added an abort to patch_reloc to verify that  
> the relocation is in range.  I've propagated the "small" flag to all of  
> the branch functions so that...
>
>>>   tcg-i386: Simplify brcond2.
>>
>> I don't like the rewrite of brcond2.
>
> ... this patch is dropped.
>
>>>   tcg-i386: Implement setcond, movcond, setcond2.
>>
>> Not yet reviewed.
>
> Fixed, attachment 4.  Similar changes to the amd64 patch.
>


Could you please send the patches inline instead. It makes them easier
to comment. 

Please find my comments here:
- I am fine with the setcond instruction
- For the movcond instruction, is there a real use for vtrue and vfalse
  values? Most CPU actually implement a version with one value.
  Implementing it with two values moves complexity within the arch
  specific tcg code.
- Do we really want to make movcond mandatory? It can be easily
  implemented using setcond, and, or instructions.
- The cmov instruction is not supported on all i386 CPU. While it is
  unlikely that someone runs qemu-system on such a CPU, it is not
  improbable that someone runs qemu-user on such a CPU. We should
  probably provide an alternative code and a check in configure (e.g.
  trying to compile asm inline code containing a cmov instruction).
- I am not sure using xor and followed by setcc *conditionally* instead
  of a movzb after setcc is a good idea. The two instructions are 
  supposed to be executed at the same speed, and time spent in code
  generation is not negligeable.
- Pay attention to the coding style, there are a few cases of if 
  without {}.

A final comment, would it be possible to split setcond and movcond in
different patches? setcond looks ready to go, there are probably some
more concerns/comments about movcond.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net