This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Improve killed check for vgpr optimization
ClosedPublic

Authored by sebastian-ne on Jul 19 2021, 11:23 AM.

Details

Summary

The killed flag is not always set. E.g. when a variable is used in a
loop, it is never marked as killed, although it is unused in following
basic blocks. Also, we try to deprecate kill flags and not use them.

Check if the register is live in the endif block. If not, consider it
killed in the then and else blocks.

The vgpr-liverange tests have two new tests with loops
(pre-committed, so the diff is visible).

Diff Detail

Event Timeline

sebastian-ne created this revision.Jul 19 2021, 11:23 AM
sebastian-ne requested review of this revision.Jul 19 2021, 11:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 11:23 AM

Kill flags are deprecated and we should try to avoid using them. Register liveness should start at the bottom of the block and scan backwards for defs

llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp
246

Shouldn't this give the same result even if it is killed? Do you really need to check it?

Kill flags are deprecated and we should try to avoid using them.

Is there any previous discussion about this? what's the motivation for this?

Register liveness should start at the bottom of the block and scan backwards for defs

After some further thinking, I agree the kill flag is not much useful. Even without kill flag, we can still easily find the kill instruction through the Kills in VarInfo with LiveVariables information if we need it.
Luckily we do not need that anymore after removing this only occurrence of checking kill flag.

llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp
246

I think Matt is correct. we just need to check the register is not live into the EndIf Block, then we can say it is killed in the Else region. The reason it works is because we only care the value is killed in the Else region, we don't care the specific instruction that kills the value. Please also update the commit message.

Kill flags are deprecated and we should try to avoid using them.

Is there any previous discussion about this? what's the motivation for this?

It's scattered around various review threads. Basically maintaining them is painful and a constant source of verifier errors. A few places still rely on them being accurate. You don't need them if you always analyze liveness looking backwards instead of forwards

sebastian-ne edited the summary of this revision. (Show Details)Jul 20 2021, 1:00 AM
sebastian-ne marked an inline comment as done.

Remove usage of kill flag and rely on liveness only.

ruiling accepted this revision.Jul 20 2021, 4:19 PM

LGTM with one nit.

llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp
242

I think the comment should be "killed in the else branch" or you can also use "killed in the else region". AFAIK kill in LLVM means the value is dead after some program point.

This revision is now accepted and ready to land.Jul 20 2021, 4:19 PM
This revision was landed with ongoing or failed builds.Jul 21 2021, 6:25 AM
This revision was automatically updated to reflect the committed changes.
sebastian-ne marked an inline comment as done.