Re: [Qemu-devel] [PATCH 0/5] tcg conditional set, round 4

[Aurelien Jarno]

On Sun, Dec 20, 2009 at 06:00:48PM -0800, Richard Henderson wrote:
> On 12/20/2009 02:57 PM, Paul Brook wrote:
>> On Saturday 19 December 2009, Richard Henderson wrote:
>>> Changes from round 3:
>>>
>>>   * Drop movcond for now.
>>>   * Only use movzbl and not xor in setcond.
>>
>> I'm still catching up on mail backlog from this thread, but I'm concerned 
>> that
>> we're exposing setcond to the target translation code if we're planning on
>> implementing movcond later.
>
> I personally was only planning to use setcond in those cases where the  
> target actually wants a 0/1 value.  E.g. i386 setcc, or alpha cmp*, or  
> mips slt*.
>
> As for shifts and masks, that wasn't in my plans.  I had a comment in  
> there about all the tricks that gcc plays with a conditional move of two  
> constants, and the fact that I didn't think it worth the effort.
>
> Indeed, my plan for today was to cut down my existing movcond patch to  
> make it simpler, as that seems to be the way Aurelien wants this to go.  
> Life conspired against and I got nothing done, but still.
>
> I *am* convinced that to remove either VTRUE or VFALSE as arguments to  
> the movcond primitive (implementing dest = (cond ? vtrue : dest) would  
> do too much violence to both the liveness analysis and the register  
> allocator within TCG.  The best I can do to remove the complexity is:
>
> static void tcg_out_movcond(TCGContext *s, int cond, TCGArg dest,
>                             TCGArg cmp1, TCGArg cmp2, int const_cmp2,
>                             TCGArg vtrue, int rexw)
> {
>     tcg_out_cmp(s, cond, cmp1, cmp2, const_cmp2, rexw);
>     /* cmovcc */
>     tcg_out_modrm(s, 0x40 | tcg_cond_to_jcc[cond] | P_EXT | rexw,
>                   dest, vtrue);
> }
> ...
>     { INDEX_op_movcond_i32, { "r", "r", "ri", "r", "0" } },
>     { INDEX_op_movcond_i64, { "r", "r", "re", "r", "0" } },
>
> using matching constraints in the target and directly implement the  
> conditional move.  This eliminates the code I previously had that  
> checked for things like dest=vfalse and inverted the condition.  I had  
> planned to simplify the i386 version similarly, even in the case where  
> cmovcc is not available.
>
> I would appreciate some direction here, so as to avoid wasting my time  
> with a solution that won't be accepted.
>

As Paul pointed, there is some redundancy between setcond and movcond,
the first one being a subset of the second one. I am not personally
convinced of the gain of both these new instructions on the performance
side, but at least I am convinced that setcond makes code in targets 
much simpler compared to brcond and jumps.

I have also very bad experience in generating highly optimized code, as
the code generation then becomes complex, which often results in a slow
down. It's clearly a fearing I have for movcond, but again I haven't
seen any benchmark going in one or the other direction. On the other 
hand setcond implementation for the various hosts is much simpler, even
on 32-bit hosts.

That said, we have had various setcond implementations discussed on the
mailing list for more than a year, and we haven't made any progress
on that, while it is an item from Fabrice's TODO list.

My proposal would be to drop movcond for now (meaning at least until
after next release to answer Paul), unless you can convince us that
movcond can bring a big gain on performance. This way we can focus
on setcond, have implementations for all hosts, and get it used in all
targets. After the next release or later, when setcond is really used
wherever possible, we can try to see if movcond is really necessary or
not.

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