thr3ads.net - llvm dev - [LLVMdev] [PATCH] [MachineSinking] Conservatively clear kill flags after coalescing. [Sep 2014]

If this information is useful, please help other people find it:
Share via:

Patrik Hägglund H

2014-Sep-05 08:03 UTC

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

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] 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<mailto: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<mailto:LLVMdev at cs.uiuc.edu>        
http://llvm.cs.uiuc.edu<http://llvm.cs.uiuc.edu/>
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140905/e66a1adf/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-MachineSinking-Conservatively-clear-kill-flags-after.patch
Type: application/octet-stream
Size: 1517 bytes
Desc: 0001-MachineSinking-Conservatively-clear-kill-flags-after.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140905/e66a1adf/attachment.obj>

Quentin Colombet

2014-Sep-05 16:39 UTC

head link

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

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] 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 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-dev/attachments/20140905/80ba9534/attachment.html>

Juergen Ributzka

2014-Sep-05 17:21 UTC

head link

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

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-dev/attachments/20140905/695a3710/attachment.html>

Quentin Colombet

2014-Sep-05 17:35 UTC

head link

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

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>

Reasonably Related Threads

Search for more possibly parallel threads

llvm dev - Sep 2014 - [LLVMdev] [PATCH] [MachineSinking] Conservatively clear kill flags after coalescing.

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

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

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

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

Reasonably Related Threads