This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Do not reassign spilled registers
ClosedPublic

Authored by rampitec on Jan 26 2021, 3:55 PM.

Details

Summary

We cannot call LRM::unassign() if LRM::assign() was never called
before, these are symmetrical calls. There are two ways of
assigning a physical register to virtual, via LRM::assign() and
via VRM::assignVirt2Phys(). LRM::assign() will call the VRM to
assign the register and then update LiveIntervalUnion. Inline
spiller calls VRM directly and thus LiveIntervalUnion never gets
updated. A call to LRM::unassign() then asserts about inconsistent
liveness.

We have to note that not all callers of the InlineSpiller even
have LRM to pass, RegAllocPBQP does not have it, so we cannot
always pass LRM into the spiller.

The only way to get into that spiller LRE_DidCloneVirtReg() call
is from LiveRangeEdit::eliminateDeadDefs if we split an LI.

This patch refuses to reassign a LiveInterval created by a split
to workaround the problem. In fact we cannot reassign a spill
anyway as all registers of the needed class are occupied and we
are spilling.

Fixes: SWDEV-267996

Diff Detail

Event Timeline

rampitec created this revision.Jan 26 2021, 3:55 PM
rampitec requested review of this revision.Jan 26 2021, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 3:55 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Jan 26 2021, 5:51 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
485

Can you tell if it's a spill from the VirtRegMap instead?

llvm/test/CodeGen/AMDGPU/nsa-reassign.mir
9–17

Don't need the registers section, should use the newer :class syntax

rampitec added inline comments.Jan 26 2021, 7:22 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
485

I don't see how.

llvm/test/CodeGen/AMDGPU/nsa-reassign.mir
9–17

The point of using it is to have 'preffered-register'. I.e. after greedy but before rewriter.

arsenm added inline comments.Jan 26 2021, 7:37 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
485

Does Register::isStackSlot return true here, or does VRM->getStackSlot() != NO_STACK_SLOT tell you?

rampitec added inline comments.Jan 26 2021, 8:05 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
485

Both don't:

(gdb) p Register::isStackSlot((unsigned)Reg)
$12 = false
(gdb) p VRM->getStackSlot(Reg) == VirtRegMap::NO_STACK_SLOT
$11 = true

I.e. it is VRM::isAssignedReg() which is checked in the first place. If not PBQB this shall be considered an RA bug.

I have done some more debugging on it. The only way to get into that spiller HoistSpillHelper::LRE_DidCloneVirtReg() call is from LiveRangeEdit::eliminateDeadDefs if we split an LI. That lives LRM in the inconsistent state so we cannot call unassign anymore. I will change the check to VRM->getPreSplitReg(Reg), it will work, but I have absolutely no idea how to write a reliable test for it.

rampitec updated this revision to Diff 319651.Jan 27 2021, 1:07 PM
rampitec edited the summary of this revision. (Show Details)

Refuse to reassign any registers which are result of an LI split.
I doubt I can create a proper test though.

rampitec marked 2 inline comments as done.Jan 27 2021, 1:07 PM
rampitec updated this revision to Diff 319688.Jan 27 2021, 3:00 PM

Added tests with forced split.

arsenm added inline comments.Jan 27 2021, 3:07 PM
llvm/lib/Target/AMDGPU/GCNNSAReassign.cpp
194

Typo incosistent

llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
474

I don't understand if this is a bug with LRM::assign or not

475

Typo incosistent

rampitec updated this revision to Diff 319691.Jan 27 2021, 3:12 PM
rampitec marked 2 inline comments as done.

Fixed typo in comments.

rampitec added inline comments.Jan 27 2021, 3:14 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
474

Not in the LRM. This is probably a bug, but in the implementation of the InlineSpiller which uses VRM directly and dies not call LRM::assign(). Nobody has hit it before because nobody tried to use unassign() after greedy I suppose. However, this is not really easy to fix it in the spiller.

arsenm added inline comments.Jan 27 2021, 3:15 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
474

Can you add a fixme explaining this and file a bug for this

rampitec marked an inline comment as done.Jan 27 2021, 3:44 PM
arsenm accepted this revision.Jan 27 2021, 4:12 PM
This revision is now accepted and ready to land.Jan 27 2021, 4:12 PM
This revision was automatically updated to reflect the committed changes.