[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] tcg conditional set/move, round 3
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] tcg conditional set/move, round 3 |
Date: |
Sat, 19 Dec 2009 14:03:46 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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
- Re: [Qemu-devel] [PATCH 1/6] tcg: Generic support for conditional set and conditional move., (continued)
- [Qemu-devel] [PATCH 5/6] tcg-i386: Simplify brcond2., Richard Henderson, 2009/12/17
- [Qemu-devel] Re: [PATCH 0/6] tcg conditional set/move, round 2, Laurent Desnogues, 2009/12/18
- [Qemu-devel] tcg conditional set/move, round 3, Richard Henderson, 2009/12/18
- Re: [Qemu-devel] tcg conditional set/move, round 3, Andreas Färber, 2009/12/19
- Re: [Qemu-devel] tcg conditional set/move, round 3,
Aurelien Jarno <=
- Re: [Qemu-devel] tcg conditional set/move, round 3, Aurelien Jarno, 2009/12/19
- Re: [Qemu-devel] tcg conditional set/move, round 3, Richard Henderson, 2009/12/19
- Re: [Qemu-devel] tcg conditional set/move, round 3, Aurelien Jarno, 2009/12/19