Page MenuHomePhabricator

RegAllocFast: Set MayLiveAcrossBlocks when allocating uses
ClosedPublic

Authored by arsenm on May 22 2019, 5:10 AM.

Details

Reviewers
qcolombet
MatzeB
Summary

Setting mayLiveOut based only on use instructions after allocating the
def block did not work if the use block was allocated before the def
block, since the virtual register uses were already removed.

Fixes bug 41973.

Diff Detail

Event Timeline

arsenm created this revision.May 22 2019, 5:10 AM

Thanks for the quick fix for bug 41973!

lib/CodeGen/RegAllocFast.cpp
295

Are you expecting future uses of this to want to use the return value? Looks like it's currently unused, and removing it would simplify some things.

arsenm marked an inline comment as done.May 22 2019, 11:57 AM
arsenm added inline comments.
lib/CodeGen/RegAllocFast.cpp
295

Possibly? The set of rewrite patches inverts the spill logic to restore on use, so these may swap but I haven't tried to rebase it yet

qcolombet added inline comments.May 24 2019, 2:54 PM
lib/CodeGen/RegAllocFast.cpp
297

We should return !MBB->pred_empty()

305

Ditto

310

Refactoring suggestion:
Use the same base code for both live-in and live-out the difference being:

  1. The returned value for early exits (pred_empty vs such_empty)
  2. The list we iterate on (defs vs uses)
qcolombet added inline comments.May 24 2019, 2:55 PM
test/CodeGen/X86/bug41973.ll
2 ↗(On Diff #200706)

BTW we should probably go with a .mir test if at all possible to be sure the layout of the use and basic blocks remains the same in the future. I.e., that this test keeps testing what it was aimed for.

arsenm marked an inline comment as done.May 24 2019, 2:59 PM
arsenm added inline comments.
lib/CodeGen/RegAllocFast.cpp
297

Does it matter for an unreachable block?

qcolombet added inline comments.May 24 2019, 3:25 PM
lib/CodeGen/RegAllocFast.cpp
297

It could be the entry block, couldn't it?

arsenm updated this revision to Diff 201378.May 24 2019, 7:14 PM
arsenm marked 2 inline comments as done.

Check pred_empty, use mir test

lib/CodeGen/RegAllocFast.cpp
297

This doesn't really matter because the entry block is always visited first

310

I tried this, but largely due to the return inside the block, it gets pretty ugly

qcolombet accepted this revision.May 27 2019, 9:13 AM

LGTM.
Nitpicks on the test below.

lib/CodeGen/RegAllocFast.cpp
310

Thanks for trying.

test/CodeGen/X86/bug41973.mir
2

Could you give a more descriptive name (regalloc-fast-missing-live-out-spill) to the file and list the PR number in the test?
Also add a description of what are the characteristic of the test (use appears before def in different blocks).

This revision is now accepted and ready to land.May 27 2019, 9:13 AM
arsenm closed this revision.May 27 2019, 1:36 PM
arsenm marked an inline comment as done.

r361781