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

[Quentin Colombet]

On Sep 5, 2014, at 10:35 AM, Quentin Colombet <qcolombet at apple.com> wrote:

> 
> 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.

I mean, we *may* extend, but we do not really know.

> 
> 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>
>>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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