llvm dev - Jun 2011 - [LLVMdev] MachineSink and EFLAGS

Hi Bill.

Thank you very much! Now I see my understanding was incorrect :) A dependence
from a single physreg-defining instruction (like CMP or TEST) is allowed to be
shared in several instructions unless that register is not clobbered (and this
is what we have with CMOV_FR64). Wouldn't it be safe then to not set the
live-in flag in EmitLoweredSelect for instructions which are marked as defining
EFLAGS (like the integer pseudo cmovs)?

Thanks,
Sergey

-----Original Message-----
From: Bill Wendling [mailto:wendling at apple.com] 
Sent: Thursday, June 02, 2011 12:00 AM
To: Galanov, Sergey
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] MachineSink and EFLAGS

On Jun 1, 2011, at 9:18 AM, Galanov, Sergey wrote:
> Hello.
>  
> I am not sure this is the right place to ask but here is my question. 
> About a year ago there was a fix of some obscure bug
(rdar://problem/8030636 which is located on the internal Apple bugtracker I
believe and so not available to the general public J) Some discussion can be
found here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100531/102160.html.
Unfortunately, no real testcase is provided, just abstract scenario description.
>  
> Basically a EFLAGS-clobbering instruction is not sunk if EFLAGS might be
live out of the current block and since a conditional branch doesn't mark
its EFLAGS use as a kill, that effectively means it is never sunk. But how can
that happen? I think the only way for it is when EFLAGS def and use are located
in different blocks but that is impossible with current instruction selector. Or
am I wrong?
>  
> A little later another fix has been made
(http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100614/102554.html)
which restricts the problem only to cases when the blocks are produced by
lowering a SELECT instruction. It is even more confusing. How is it different
from the other scenario wrt. EFLAGS use? Moreover, pseudo cmov is marked as
clobbering EFLAGS so how can it be used further in the code?
>  
> Are there plans to implement a less conservative fix? I believe proper phys
regs liveness information is required for that.
>  
Hi Sergey,

I was the one who created the patch. Unfortunately, there wasn't a testcase
that I could use that was small and wouldn't require modification after
every compiler change.

Looking at the bug report, I've attached the testcase below. You can compile
it like this:

$ llvm-gcc -Os -fmessage-length=0 -pipe -std=gnu99 -fpascal-strings -fasm-blocks
-pedantic -msse3 test.c -o test.s -S

See the attached test.s for comments on what is wrong with the output.

You're correct that proper physical register liveness information is
required for a less conservative fix. However, I don't know if that will
ever happen. If it does, then we can make this pass more liberal.

-bw


--------------------------------------------------------------------
Closed Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park, 
17 Krylatskaya Str., Bldg 4, Moscow 121614, 
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

On Jun 2, 2011, at 3:53 AM, Galanov, Sergey wrote:
> Hi Bill.
> 
> Thank you very much! Now I see my understanding was incorrect :) A
dependence from a single physreg-defining instruction (like CMP or TEST) is
allowed to be shared in several instructions unless that register is not
clobbered (and this is what we have with CMOV_FR64). Wouldn't it be safe
then to not set the live-in flag in EmitLoweredSelect for instructions which are
marked as defining EFLAGS (like the integer pseudo cmovs)?
> 
Hi Sergey,

I'm not certain what that would buy us. If I understand your suggestion
correctly, not setting the EFLAGS as live-in to a basic block would mean that
we'd have to execute the instruction to reset the EFLAGS register, right?
That would mean more code, longer live ranges, etc.

-bw

On Jun 2, 2011, at 12:58 PM, Bill Wendling wrote:
> On Jun 2, 2011, at 3:53 AM, Galanov, Sergey wrote:
> 
>> Hi Bill.
>> 
>> Thank you very much! Now I see my understanding was incorrect :) A
dependence from a single physreg-defining instruction (like CMP or TEST) is
allowed to be shared in several instructions unless that register is not
clobbered (and this is what we have with CMOV_FR64). Wouldn't it be safe
then to not set the live-in flag in EmitLoweredSelect for instructions which are
marked as defining EFLAGS (like the integer pseudo cmovs)?
>> 
> Hi Sergey,
> 
> I'm not certain what that would buy us. If I understand your suggestion
correctly, not setting the EFLAGS as live-in to a basic block would mean that
we'd have to execute the instruction to reset the EFLAGS register, right?
That would mean more code, longer live ranges, etc.
To clarify, a physical register may be defined in one basic block and used in
another. In that case, it must be marked as live-in in all basic blocks entered
on any path between the def and the use.

This is effectively manual register allocation, and it should be avoided. It is
necessary sometimes, though. Like in select lowering.

The machine code verifier has good checks for this. If it complains about
missing live-in registers, miscompilations are possible.

/jakob