This is an archive of the discontinued LLVM Phabricator instance.

Fix LivePhysRegs::addLiveOuts
ClosedPublic

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

Details

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.
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 subscriber: mcrosier.

Test case?

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

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

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

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?

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())

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
This revision was automatically updated to reflect the committed changes.