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

[Juergen Ributzka]

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.

-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. /Patrik Hä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>
> >  
> > MachineVerifier then 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 --git a/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 @@ bool MachineSinking::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 <llvmdev-bounces at cs.uiuc.edu>] On Behalf Of Quentin 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 on llvm.org/bugs(http://llvm.org/bugs) with 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-commits/attachments/20140905/695a3710/attachment.html>