[LLVMdev] [PATCH] [MachineSinking] Conservatively clear kill flags after coalescing.

[Quentin Colombet]

On Sep 5, 2014, at 10:21 AM, Juergen Ributzka <juergen at apple.com> wrote:

> clearKillFlags seems a little "overkill" to me. In this case you could just simply transfer the value of the kill flag from the SrcReg to the DstReg.

We are extending the live-range of SrcReg. I do not see how you could relate that to the kill flag of DstReg.

Therefore, I still think, this is the right fix.

-Quentin

> 
> -Juergen
> 
> On 09/05/14, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> Hi Patrik,
>> 
>> 
>> LGTM.
>> 
>> Thanks,
>> -Quentin
>> On Sep 5, 2014, at 1:03 AM, Patrik Hägglund H <patrik.h.hagglund at ericsson.com> wrote:
>> 
>>> Hi Quentin,
>>> 
>>> Jonas looked further into the problem below, and asked me to submit his patch. Note the we have our own out-of-tree target, and we have not been able to reproduce this problem on an in-tree target. /PatrikHägglund
>>> 
>>> [MachineSinking] Conservatively clear kill flags after coalescing.
>>> 
>>> This solves the problem of having a kill flag inside a loop
>>> with a definition of the register prior to the loop:
>>> 
>>> %vreg368<def> ...
>>> 
>>> Inside loop:
>>> 
>>>        %vreg520<def> = COPY %vreg368
>>>        %vreg568<def,tied1> = add %vreg341<tied0>, %vreg520<kill>
>>> 
>>> => was coalesced into =>
>>> 
>>>        %vreg568<def,tied1> =add%vreg341<tied0>, %vreg368<kill>
>>> 
>>> MachineVerifierthen complained:
>>> *** Bad machine code: Virtual register killed in block, but needed live out. ***
>>> 
>>> The kill flag for %vreg368 is incorrect, and is cleared by this patch.
>>> 
>>> This is similar to the clearing done at the end of
>>> MachineSinking::SinkInstruction().
>>> ---
>>> lib/CodeGen/MachineSink.cpp | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>> 
>>> diff --gita/lib/CodeGen/MachineSink.cpp b/lib/CodeGen/MachineSink.cpp
>>> index 7782001..261af54 100644
>>> --- a/lib/CodeGen/MachineSink.cpp
>>> +++ b/lib/CodeGen/MachineSink.cpp
>>> @@ -157,6 +157,11 @@boolMachineSinking::PerformTrivialForwardCoalescing(MachineInstr*MI,
>>>   DEBUG(dbgs() << "*** to: " << *MI);
>>>   MRI->replaceRegWith(DstReg,SrcReg);
>>>   MI->eraseFromParent();
>>> +
>>> + // Conservatively, clear any kill flags, since it's possible that they are no
>>> + // longer correct.
>>> + MRI->clearKillFlags(SrcReg);
>>> +
>>>   ++NumCoalesces;
>>>   return true;
>>> }
>>> --
>>> 2.1.0
>>> 
>>> From:llvmdev-bounces at cs.uiuc.edu[mailto:llvmdev-bounces at cs.uiuc.edu]On Behalf OfQuentin Colombet
>>> Sent:den 2 september 2014 18:37
>>> To:Jonas Paulsson
>>> Cc:llvmdev at cs.uiuc.edu
>>> Subject:Re: [LLVMdev] Machine code sinking pass
>>>  
>>> Hi Jonas,
>>> 
>>> This looks like a bug in Machine code sinking pass.
>>> 
>>> Please file a PR onllvm.org/bugswith a reduced test case to keep track of the issue.
>>> 
>>> Thanks,
>>> -Quentin
>>> 
>>> On Sep 2, 2014, at 5:57 AM, Jonas Paulsson <jonas.paulsson at ericsson.com> wrote:
>>> 
>>> 
>>> Hi,
>>> I ran into MachineVerifier ”Virtual register killed in block, but needed live out.”
>>>  
>>> It was MachineSinking:PerformTrivialForwardCoalescing() that coalesced a COPY inside a single-block loop, but left the
>>> kill-flag and then MachineVerifier complains that a register in vregsRequired is killed in MBB.
>>>  
>>> In the example, %vreg520 is replaced by %vreg368 by PerformTrivialForwardCoalescing(), without clearing the kill flag:
>>>  
>>> BB#13: derived from LLVM BB %CF250
>>>     Predecessors according to CFG: BB#12 BB#13 BB#14
>>>        …
>>>         %vreg520<def> = COPY %vreg368
>>>         %vreg568<def,tied1> = cmp %vreg341<tied0>, %vreg520<kill>
>>>        brr_cond <BB#13>
>>>         brr_uncond <BB#14>
>>>     Successors according to CFG: BB#13, BB#14
>>>  
>>> Into
>>>  
>>> BB#13: derived from LLVM BB %CF250
>>>     Predecessors according to CFG: BB#12 BB#13 BB#14
>>>         …
>>>         %vreg568<def,tied1> = cmp %vreg341<tied0>, %vreg368<kill>
>>>        brr_cond <BB#13>
>>>         brr_uncond <BB#14>
>>> =>
>>> *** Bad machine code: Virtual register killed in block, but needed live out. ***
>>> - function:    autogen_SD15028
>>> - basic block: BB#13 CF250 (0x1c75890)
>>> Virtual register %vreg368 is used after the block.
>>> There is only one use of %vreg368 in the function.
>>>  
>>> One thing that strikes me is that one might want to clear the kill flag for a use inside a loop of a register defined prior to
>>> the loop?
>>>  
>>> Best regards,
>>>  
>>> Jonas Paulsson
>>>  
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> LLVMdev at cs.uiuc.edu        http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>> 
>>> <0001-MachineSinking-Conservatively-clear-kill-flags-after.patch>
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140905/d5165f94/attachment.html>