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
aurel...@aurel32.net http://www.aurel32.net
