This is an archive of the discontinued LLVM Phabricator instance.

Differential D16434

Fix LivePhysRegs::addLiveOuts
ClosedPublic

Authored by weimingz on Jan 21 2016, 4:13 PM.

Details

Reviewers
t.p.northover
MatzeB
apazos
Commits
rG13e7cb294c95: Fix LivePhysRegs::addLiveOuts
rL258569: Fix LivePhysRegs::addLiveOuts
Summary

The testing for returnBB was flipped which may cause ARM ld/st opt pass uses callee saved regs in returnBB when shrink-wrap is used.

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz updated this revision to Diff 45611.Jan 21 2016, 4:13 PM
weimingz retitled this revision from to Fix ARM load/store opt live reg computing.
weimingz updated this object.
weimingz added a reviewer: t.p.northover.
weimingz set the repository for this revision to rL LLVM.
weimingz added a subscriber: llvm-commits.
Herald added subscribers: rengolin, aemerson. · View Herald TranscriptJan 21 2016, 4:13 PM
weimingz added a comment.Jan 21 2016, 4:13 PM
E.g.:
Before LD/ST opt:
BB1:
  ...
  %SP<def,tied1> = t2LDMIA_UPD %SP<tied0>, pred:14, pred:%noreg,
                   %R4<def>, %R5<def>, %R7<def>, %LR<def>
  Successors according to CFG: BB#3

BB#3: derived from LLVM BB %if.end
    Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
    t2STRi12 %R2<kill>, %R1, 20, pred:14, pred:%noreg;
    t2STRi12 %R3<kill>, %R1, 24, pred:14, pred:%noreg; mem:ST4[%22]
    t2STRi12 %R12<kill>, %R1, 28, pred:14, pred:%noreg; mem:ST4[%23]
    tBX_RET pred:14, pred:%noreg

After LD/ST opt:
BB#3: derived from LLVM BB %if.end
    Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
    %LR<def> = t2ADDri %R1, 20, pred:14, pred:%noreg, opt:%noreg
    t2STMIA %LR<kill>, pred:14, pred:%noreg, %R2<kill>, %R3<kill>, %R12<kill>
    tBX_RET pred:14, pred:%noreg
zzheng added a subscriber: zzheng.Jan 21 2016, 4:16 PM

Looks like a typo.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
564 ↗(On Diff #45611)

Same test on both sides of '||' ?

weimingz added inline comments.Jan 21 2016, 4:38 PM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
564 ↗(On Diff #45611)

yes, it's typo. Thanks!

weimingz updated this revision to Diff 45615.Jan 21 2016, 4:38 PM
weimingz added a reviewer: apazos.
mcrosier added a reviewer: MatzeB.Jan 22 2016, 9:33 AM
mcrosier added a subscriber: mcrosier.

Test case?

weimingz added a comment.Jan 22 2016, 10:25 AM
In D16434#333602, @mcrosier wrote:

Test case?

Unfortunately, it's hard to reduce to a simple test case because

  1. The reproduce the issue, we need to make sure certain regs are used at that time
  2. The problem is exposed using a modified reg alloc policy which is not available on community now.
MatzeB requested changes to this revision.Jan 22 2016, 10:39 AM
MatzeB edited edge metadata.

This patch looks like fixing a symptom rather than the root cause. Shouldn't BX_RET simply have an implicit use operand for LR? So this would rather need fixing where BX_RET is constructed.

This revision now requires changes to proceed.Jan 22 2016, 10:39 AM
weimingz added a comment.Jan 22 2016, 10:51 AM
In D16434#333667, @MatzeB wrote:

This patch looks like fixing a symptom rather than the root cause. Shouldn't BX_RET simply have an implicit use operand for LR? So this would rather need fixing where BX_RET is constructed.

Yes, I tried to add a use in td file like:
let isReturn = 1, isTerminator = 1, isBarrier = 1, Uses = [LR] in {

def tBX_RET : tPseudoExpand<(outs), (ins pred:$p), 2, IIC_Br,
                 [(ARMretflag)], (tBX LR, pred:$p)>, Sched<[WriteBr]>;

Then I got an error like "use of undefined register LR". Any suggestions?

Thanks

MatzeB added a comment.Jan 22 2016, 10:55 AM
In D16434#333738, @weimingz wrote:
In D16434#333667, @MatzeB wrote:

This patch looks like fixing a symptom rather than the root cause. Shouldn't BX_RET simply have an implicit use operand for LR? So this would rather need fixing where BX_RET is constructed.

Yes, I tried to add a use in td file like:
let isReturn = 1, isTerminator = 1, isBarrier = 1, Uses = [LR] in {

def tBX_RET : tPseudoExpand<(outs), (ins pred:$p), 2, IIC_Br,
                 [(ARMretflag)], (tBX LR, pred:$p)>, Sched<[WriteBr]>;

Then I got an error like "use of undefined register LR". Any suggestions?

I assume you removed LR from the callee saved register set, in that case it probably has to go into the live-in list in the start block.

weimingz updated this revision to Diff 45734.Jan 22 2016, 1:26 PM
weimingz updated this object.
weimingz edited edge metadata.

At first, I thought LR was just a corner case that ld/st opt didn't handled correctly.
Then, I find another case where R4 is used in exiting BB and not restored.

Before ld/st opt pass:

BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

t2STRi12 %R2<kill>, %R1, 20, pred:14, pred:%noreg; mem:ST4[%21]
t2STRi12 %R3<kill>, %R1, 24, pred:14, pred:%noreg; mem:ST4[%22]
t2STRi12 %R12<kill>, %R1, 28, pred:14, pred:%noreg; mem:ST4[%23]
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

After ld/st opt pass:
BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

%R4<def> = t2ADDri %R1, 20, pred:14, pred:%noreg, opt:%noreg
t2STMIA %R4<kill>, pred:14, pred:%noreg, %R2<kill>, %R3<kill>, %R12<kill> ==> R4 is clobbed
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

MatzeB added a comment.Jan 22 2016, 1:30 PM
In D16434#333962, @weimingz wrote:

At first, I thought LR was just a corner case that ld/st opt didn't handled correctly.
Then, I find another case where R4 is used in exiting BB and not restored.

Before ld/st opt pass:

BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

t2STRi12 %R2<kill>, %R1, 20, pred:14, pred:%noreg; mem:ST4[%21]
t2STRi12 %R3<kill>, %R1, 24, pred:14, pred:%noreg; mem:ST4[%22]
t2STRi12 %R12<kill>, %R1, 28, pred:14, pred:%noreg; mem:ST4[%23]
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

After ld/st opt pass:
BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

%R4<def> = t2ADDri %R1, 20, pred:14, pred:%noreg, opt:%noreg
t2STMIA %R4<kill>, pred:14, pred:%noreg, %R2<kill>, %R3<kill>, %R12<kill> ==> R4 is clobbed
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

But LivePhysRegs::addLiveOuts should add all the callee saves! Does maybe MBB->isReturnBlock() return false somehow in your case?

weimingz added a comment.Jan 22 2016, 1:43 PM
In D16434#333966, @MatzeB wrote:
In D16434#333962, @weimingz wrote:

At first, I thought LR was just a corner case that ld/st opt didn't handled correctly.
Then, I find another case where R4 is used in exiting BB and not restored.

Before ld/st opt pass:

BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

t2STRi12 %R2<kill>, %R1, 20, pred:14, pred:%noreg; mem:ST4[%21]
t2STRi12 %R3<kill>, %R1, 24, pred:14, pred:%noreg; mem:ST4[%22]
t2STRi12 %R12<kill>, %R1, 28, pred:14, pred:%noreg; mem:ST4[%23]
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

After ld/st opt pass:
BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

%R4<def> = t2ADDri %R1, 20, pred:14, pred:%noreg, opt:%noreg
t2STMIA %R4<kill>, pred:14, pred:%noreg, %R2<kill>, %R3<kill>, %R12<kill> ==> R4 is clobbed
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

But LivePhysRegs::addLiveOuts should add all the callee saves! Does maybe MBB->isReturnBlock() return false somehow in your case?

Oh. currently in addLiveOuts:

if (!MBB->isReturnBlock()) {
  // The return block has no successors whose live-ins we could merge
  // below. So instead we add the callee saved registers manually.
  for (const MCPhysReg *I = TRI->getCalleeSavedRegs(&MF); *I; ++I)
    addReg(*I);

Looks we should flip the condition to
if (!MBB->isReturnBlock())

MatzeB added a comment.Jan 22 2016, 1:44 PM
In D16434#333985, @weimingz wrote:
In D16434#333966, @MatzeB wrote:
In D16434#333962, @weimingz wrote:

At first, I thought LR was just a corner case that ld/st opt didn't handled correctly.
Then, I find another case where R4 is used in exiting BB and not restored.

Before ld/st opt pass:

BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

t2STRi12 %R2<kill>, %R1, 20, pred:14, pred:%noreg; mem:ST4[%21]
t2STRi12 %R3<kill>, %R1, 24, pred:14, pred:%noreg; mem:ST4[%22]
t2STRi12 %R12<kill>, %R1, 28, pred:14, pred:%noreg; mem:ST4[%23]
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

After ld/st opt pass:
BB#3: derived from LLVM BB %if.end

Live Ins: %R0 %R1 %R2 %R3 %R12 %LR %R7 %R5 %R4
Predecessors according to CFG: BB#2 BB#1

%R4<def> = t2ADDri %R1, 20, pred:14, pred:%noreg, opt:%noreg
t2STMIA %R4<kill>, pred:14, pred:%noreg, %R2<kill>, %R3<kill>, %R12<kill> ==> R4 is clobbed
t2STRi12 %R0<kill>, %R1<kill>, 32, pred:14, pred:%noreg; mem:ST4[%24]
tBX_RET pred:14, pred:%noreg

But LivePhysRegs::addLiveOuts should add all the callee saves! Does maybe MBB->isReturnBlock() return false somehow in your case?

Oh. currently in addLiveOuts:

if (!MBB->isReturnBlock()) {
  // The return block has no successors whose live-ins we could merge
  // below. So instead we add the callee saved registers manually.
  for (const MCPhysReg *I = TRI->getCalleeSavedRegs(&MF); *I; ++I)
    addReg(*I);

Looks we should flip the condition to
if (!MBB->isReturnBlock())

Oh, right. I'm surprised nobody has hit a bug because of this before.

weimingz updated this revision to Diff 45741.Jan 22 2016, 2:09 PM
weimingz retitled this revision from Fix ARM load/store opt live reg computing to Fix LivePhysRegs::addLiveOuts.
weimingz updated this object.
weimingz edited edge metadata.
weimingz removed rL LLVM as the repository for this revision.

I think originally the problem was covered because epilogue will do the job. Now with shrink-wrap, it is exposed.

MatzeB accepted this revision.Jan 22 2016, 2:10 PM
MatzeB edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 22 2016, 2:10 PM
weimingz closed this revision.Jan 22 2016, 2:25 PM
Closed by commit rL258569: Fix LivePhysRegs::addLiveOuts (authored by weimingz). · Explain WhyJan 22 2016, 2:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
CodeGen/
2 lines

Diff 45744

llvm/trunk/lib/CodeGen/LivePhysRegs.cpp

Show First 20 Lines • Show All 145 Lines • ▼ Show 20 Lines for (const CalleeSavedInfo &Info : MFI.getCalleeSavedInfo())
LiveRegs.removeReg(Info.getReg()); LiveRegs.removeReg(Info.getReg());
} }
void LivePhysRegs::addLiveOuts(const MachineBasicBlock *MBB, void LivePhysRegs::addLiveOuts(const MachineBasicBlock *MBB,
bool AddPristinesAndCSRs) { bool AddPristinesAndCSRs) {
if (AddPristinesAndCSRs) { if (AddPristinesAndCSRs) {
const MachineFunction &MF = *MBB->getParent(); const MachineFunction &MF = *MBB->getParent();
addPristines(*this, MF, *TRI); addPristines(*this, MF, *TRI);
if (!MBB->isReturnBlock()) { if (MBB->isReturnBlock()) {
// The return block has no successors whose live-ins we could merge // The return block has no successors whose live-ins we could merge
// below. So instead we add the callee saved registers manually. // below. So instead we add the callee saved registers manually.
for (const MCPhysReg *I = TRI->getCalleeSavedRegs(&MF); *I; ++I) for (const MCPhysReg *I = TRI->getCalleeSavedRegs(&MF); *I; ++I)
addReg(*I); addReg(*I);
} }
} }
// To get the live-outs we simply merge the live-ins of all successors. // To get the live-outs we simply merge the live-ins of all successors.
Show All 12 Lines