Page MenuHomePhabricator

[LivePhysRegs] Fix handling of return instructions.
ClosedPublic

Authored by efriedma on Jan 29 2018, 12:23 PM.

Details

Summary

See D42509 for the original version of this.

Basically, there are two significant changes to behavior here:

  1. addLiveOuts always adds all pristine registers (even if a block has no successors).
  2. addLiveOuts and addLiveOutsNoPristines always add all callee-saved registers for return blocks (including conditional return blocks).

I cleaned up the functions a bit to make it clear these properties hold.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Jan 29 2018, 12:23 PM
Andi added a subscriber: Andi.Jan 30 2018, 7:51 AM
MatzeB accepted this revision.Jan 30 2018, 5:01 PM

Argh:

...
    BX_RET 4, killed %cpsr, implicit %r0
    Successors according to CFG: %bb.2(0x40000000 / 0x80000000 = 50.00%)
...

A conditional return statement breaks the whole nice logic of MachineBasicBlock::isReturnBlock() returning true or false, since it's clearly neither in this case...

LLVM CodeGen isn't really prepared for predicated code (= instructions that don't necessarily execute) it seemed fine to me when they are all packaged up in a bundle and reasoning about it as a bundle made sense, but this is turning a sure branch into a possible branch and a fallthrough...

Apart from that I have really bad feelings about us using MI this way, I'm fine to push this as a fix for now.

lib/CodeGen/LivePhysRegs.cpp
228 ↗(On Diff #131848)

Being how unintuitive all of this turned out to be this deserves a comment!

This revision is now accepted and ready to land.Jan 30 2018, 5:01 PM
kparzysz added inline comments.Jan 31 2018, 6:21 AM
lib/CodeGen/LivePhysRegs.cpp
232 ↗(On Diff #131848)

Shouldn't we be doing this even for conditional returns?

efriedma added inline comments.Jan 31 2018, 11:32 AM
lib/CodeGen/LivePhysRegs.cpp
232 ↗(On Diff #131848)

I... guess so?

I'll try to come up with something more obviously correct.

efriedma updated this revision to Diff 132241.Jan 31 2018, 12:08 PM
efriedma retitled this revision from [LivePhysRegs] Fix handling of conditional return instructions. to [LivePhysRegs] Fix handling of return instructions..
efriedma edited the summary of this revision. (Show Details)
efriedma added a reviewer: kparzysz.

Mark all callee-saved regs live in return blocks in addLiveOuts/addLiveOutsNoPristines. Clean up the code to make the behavior more obvious. Rebase patch against trunk, since the original patch was reverted. Update summary.

efriedma requested review of this revision.Jan 31 2018, 12:08 PM
efriedma marked 3 inline comments as done.
kparzysz accepted this revision.Feb 5 2018, 9:47 AM
This revision is now accepted and ready to land.Feb 5 2018, 9:47 AM
This revision was automatically updated to reflect the committed changes.