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
575

Same test on both sides of '||' ?

weimingz added inline comments.Jan 21 2016, 4:38 PM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
575

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
lib/
Target/
ARM/
11 lines

Diff 45734

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

Show First 20 Lines • Show All 547 Lines • ▼ Show 20 Lines
/// direction). Computes backwards so multiple queries in the same block must /// direction). Computes backwards so multiple queries in the same block must
/// come in reverse order. /// come in reverse order.
void ARMLoadStoreOpt::moveLiveRegsBefore(const MachineBasicBlock &MBB, void ARMLoadStoreOpt::moveLiveRegsBefore(const MachineBasicBlock &MBB,
MachineBasicBlock::const_iterator Before) { MachineBasicBlock::const_iterator Before) {
// Initialize if we never queried in this block. // Initialize if we never queried in this block.
if (!LiveRegsValid) { if (!LiveRegsValid) {
LiveRegs.init(TRI); LiveRegs.init(TRI);
LiveRegs.addLiveOuts(&MBB, true); LiveRegs.addLiveOuts(&MBB, true);
auto MI = MBB.getLastNonDebugInstr();
// For exiting BB, we need to initially mark callee saved regs as live-out
// because passes like shrink-wrap may move the reg restoring around.
if (MI != MBB.end() &&
(MI->getOpcode() == ARM::BX_RET || MI->getOpcode() == ARM::tBX_RET ||
MI->getOpcode() == ARM::MOVPCLR)) {
const MCPhysReg *CSRegs = TRI->getCalleeSavedRegs(MBB.getParent());
while (*CSRegs)
LiveRegs.addReg(*CSRegs++);
}
LiveRegPos = MBB.end(); LiveRegPos = MBB.end();
LiveRegsValid = true; LiveRegsValid = true;
} }
// Move backward just before the "Before" position. // Move backward just before the "Before" position.
while (LiveRegPos != Before) { while (LiveRegPos != Before) {
--LiveRegPos; --LiveRegPos;
LiveRegs.stepBackward(*LiveRegPos); LiveRegs.stepBackward(*LiveRegPos);
} }
} }
zzhengUnsubmitted
Not Done
ReplyInline Actions

Same test on both sides of '||' ?

zzheng: Same test on both sides of '||' ?
weimingzAuthorUnsubmitted
Not Done
ReplyInline Actions

yes, it's typo. Thanks!

weimingz: yes, it's typo. Thanks!
static bool ContainsReg(const ArrayRef<std::pair<unsigned, bool>> &Regs, static bool ContainsReg(const ArrayRef<std::pair<unsigned, bool>> &Regs,
unsigned Reg) { unsigned Reg) {
for (const std::pair<unsigned, bool> &R : Regs) for (const std::pair<unsigned, bool> &R : Regs)
if (R.first == Reg) if (R.first == Reg)
return true; return true;
return false; return false;
} }
▲ Show 20 Lines • Show All 1,766 Lines • Show Last 20 Lines