[LLVMdev] [PATCH] [MachineSinking] Conservatively clear kill flags after coalescing.
Patrik Hägglund H
patrik.h.hagglund at ericsson.com
Tue Sep 9 12:04:25 PDT 2014
Thanks! Committed as r217427.
/Patrik Hägglund
From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Juergen Ributzka
Sent: den 5 september 2014 21:01
To: Quentin Colombet
Cc: llvm-commits at cs.uiuc.edu; llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] [PATCH] [MachineSinking] Conservatively clear kill flags after coalescing.
You are right. It isn't worth the risk and effort.
Cheers,
Juergen
On 09/05/14, Quentin Colombet <qcolombet at apple.com<mailto:qcolombet at apple.com>> wrote:
On Sep 5, 2014, at 11:27 AM, Juergen Ributzka <juergen at apple.com<mailto:juergen at apple.com>> wrote:
When the SrcReg has the kill flag set, we don't want to propagate that and just keeping the original DstReg kill flag at its last use. Basically there is no work to be done when SrcReg has the kill flag set.
When the kill flag is not set we have to first clear the kill flag from all DstReg uses and then perform the replacement with the SrcReg. If we would do the clearing of the kill flag afterwards we would clear more kill flags than necessary and perform more work too.
if (!MI->getOperand(1).isKill())
MRI->clearKillFlags(DstReg);
MRI->replaceRegWith(DstReg, SrcReg);
DstReg<def> = COPY SrcReg<kill> DstReg<def> = COPY SrcReg
... = ... DstReg ... = ... DstReg
... = ... DstReg<kill> ... = ... DstReg<kill>
... = ... SrcReg<kill>
----->
... = ... SrcReg ... = ... SrcReg
... = ... SrcReg<kill> ... = ... SrcReg
... = ... SrcReg<kill> <-- this would get cleared too if we clear the kill flags for SrcReg.
Yes, but if you consider a different example where SrcReg is killed before DstReg, then it is wrong:
DstReg = COPY SrcReg
... = ... SrcReg<kill>
... = ... DstReg<kill or not>
-->
... = ... SrcReg<kill> <- this isn't cleared whereas it should.
... = ... SrcReg<kill or not>
We could indeed do some tricks to preserve the kill flags, but in my opinion, this is error prone and is not worth the risk.
Cheers,
-Quentin
-Juergen
On 09/05/14, Quentin Colombet <qcolombet at apple.com<mailto:qcolombet at apple.com>> wrote:
On Sep 5, 2014, at 10:35 AM, Quentin Colombet <qcolombet at apple.com<mailto:qcolombet at apple.com>> wrote:
On Sep 5, 2014, at 10:21 AM, Juergen Ributzka <juergen at apple.com<mailto: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<mailto: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<mailto: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>[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<mailto: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/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
<0001-MachineSinking-Conservatively-clear-kill-flags-after.patch>
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto: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/20140909/425ebc11/attachment.html>
More information about the llvm-dev
mailing list