This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Use kill instruction to hint soft clause live ranges
ClosedPublic

Authored by arsenm on Feb 15 2021, 2:04 PM.

Details

Summary

Previously we would use a bundle to hint the register allocator to not
overwrite the pointers in a sequence of loads to avoid breaking soft
clauses. This bundling was based on a fuzzy register pressure
heuristic, so we could not guarantee using more registers than are
really available. This would result in register allocator failing on
unsatisfiable bundles. Use a kill to artificially extend the live
ranges, so we can always succeed at register allocation even if it
means extra spills in the worst case.

This seems to capture most of the benefit of the bundle while avoiding
most of the risk presented by the bundle. However the lit tests do
show a handful of regressions. In some cases with sequences of
volatile loads, unused load components end up getting reallocated to
the next load which forces a wait between. There are also a few small
scheduling regressions where a hazard used to be avoided, and one
spill torture test which for some reason nearly doubles the stack
usage. There is also a bit of noise from leftover kills (it may make
sense for post-RA pseudos to strip all of these out).

Diff Detail

Event Timeline

arsenm created this revision.Feb 15 2021, 2:04 PM
arsenm requested review of this revision.Feb 15 2021, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 2:04 PM
Herald added a subscriber: wdng. · View Herald Transcript
rampitec added inline comments.Feb 16 2021, 10:22 AM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
339–341

Since it is not a bundle anymore you probably want to use name "Clause" instead of "Bundle".

344

You probably want to use a SetVector to avoid duplicate operands.

arsenm updated this revision to Diff 324124.Feb 16 2021, 3:28 PM

Use a kill per register. All-in-one kill had the same problem as the bundle (the allocator seems to not know it's OK to delete these if it can't handle it)

arsenm updated this revision to Diff 324125.Feb 16 2021, 3:29 PM

Rename variable

foad added a comment.Feb 17 2021, 12:28 AM

This would result in register allocator failing on
unsatisfiable bundles.

Test case for that?

This would result in register allocator failing on
unsatisfiable bundles.

Test case for that?

D96822, plus 1e377a273f59375d8e6a424f66f069b3adfa1ca4 had more whenever that is un-reverted

With this I do see some ridiculous situations where restores are inserted for the use on the kill

Rename variable

Matt, it is not that variable. Post-RA it is still bundles, but in SIFormMemoryClauses not anymore.

You also can have duplicate kills, I think you need to use SetVector, not SmallVector.

Rename variable

Matt, it is not that variable. Post-RA it is still bundles, but in SIFormMemoryClauses not anymore.

You also can have duplicate kills, I think you need to use SetVector, not SmallVector.

No you can't, the Uses set already uniques this

arsenm updated this revision to Diff 324433.Feb 17 2021, 2:42 PM
rampitec accepted this revision.Feb 17 2021, 4:31 PM

LGTM. You need to rebase test though, add extra 0 operand to every memory operation. Sorry about that, D96469 is not ready yet to get rid of it.

This revision is now accepted and ready to land.Feb 17 2021, 4:31 PM