This is an archive of the discontinued LLVM Phabricator instance.

[RegisterCoalescer] passs Undefs to extendToIndices()
ClosedPublic

Authored by ruiling on Sep 15 2020, 11:42 PM.

Details

Summary

When extending the subranges, the reaching-def may be an undefs. When
extending such kind of subrange, it will try to search for the reaching
def first. If the reaching def is an undef and we did not provide 'Undefs',
The findReachingDefs() will fail with message:
"Use of $noreg does not have a corresponding definition on every path:
LLVM ERROR: Use not jointly dominated by defs."
So we computeSubRangeUndefs() and pass the result to extendToIndices().

Diff Detail

Event Timeline

ruiling created this revision.Sep 15 2020, 11:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 11:42 PM
ruiling requested review of this revision.Sep 15 2020, 11:42 PM
arsenm added inline comments.Sep 16 2020, 7:07 AM
llvm/test/CodeGen/AMDGPU/coalescer-removepartial-extend-undef-subrange.mir
20

$noreg doesn't have liveness?

29

Can you reduce this any further? -run-pass=none will also compact the register numbers

Typo passs in commit message

ruiling added inline comments.Sep 16 2020, 6:34 PM
llvm/test/CodeGen/AMDGPU/coalescer-removepartial-extend-undef-subrange.mir
20

I am not sure what you are really asking here. I know the assert message is a little bit misleading. The reason it output $noreg is we are passing 0 to argument 'PhyReg' of LiveRangeCalc::extend() in LiveIntervals::extendToIndices(). When assert happens, we are trying to extend a subrange of register %90. The offending subrange has a def point in one of bb.4 predecessor chain, so it has liveness in this path. but in another predecessor chain 4->27->1->0, it only has one undef point in bb.0.

29

I just tried to remove each one of the most blocks containing instructions in MIR, I failed to trigger the crash after removing them. If you have any good idea to simplify further, I can try it. Some more notes: The test case is already a much simplified version than original failure IR with 100+ basic blocks. I have tried removing each basic block in IR before and it would cause problem disappear. The problem is hard to trigger, it also depends on the order of coalescing work. For example, if I reorder the blocks, the problem may disappear because the coalescing of registers will be different then.

ruiling updated this revision to Diff 292404.Sep 16 2020, 8:50 PM

address some review comments from Matt

arsenm added inline comments.Sep 17 2020, 7:29 AM
llvm/test/CodeGen/AMDGPU/coalescer-removepartial-extend-undef-subrange.mir
29

I've reduced coalescer cases by dumping the MIR at intermediate points during the coalescer. Presumably not every step is relevant to get to the problem

ruiling updated this revision to Diff 292685.Sep 17 2020, 5:55 PM

further reduce .mir with intermediate result from coalescer.

ruiling updated this revision to Diff 292706.Sep 17 2020, 10:31 PM

further reduce .mir to trigger the issue. @arsenm Could you take a look?

ruiling updated this revision to Diff 293650.Sep 22 2020, 11:01 PM

further simplify .mir test

arsenm accepted this revision.Sep 23 2020, 5:46 AM

LGTM with test converted to generated checks

llvm/test/CodeGen/AMDGPU/coalescer-removepartial-extend-undef-subrange.mir
2

Add a -mcpu=gfx900 or some other target

4–6

These checks are really weak, I would rather just generate these

This revision is now accepted and ready to land.Sep 23 2020, 5:46 AM
This revision was automatically updated to reflect the committed changes.