This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] avoid blind converting to VALU REG_SEQUENCE and PHIs
ClosedPublic

Authored by alex-t on Jul 22 2022, 8:15 AM.

Details

Summary

In the 2e29b0138ca243 we introduce a specific solving algorithm
that analyzes the VGPR to SGPR copies use chains and either lowers
the copy to v_readfirstlane_b32 or converts the whole chain to VALU forms.
Same time we still have the code that blindly converts to VALU REG_SEQUENCE and PHIs
in case they produce SGPR but have VGPRs input operands. In case the REG_SEQUENCE and PHIs
are in the VGPR to SGPR copy use chain, and this chain was considered long enough to convert
copy to v_readfistlane_b32, further lowering them to VALU leads to several kinds of issues.
At first, we have v_readfistlane_b32 which is completely useless because most parts of its use chain
were moved to VALU forms. Second, we may encounter subtle bugs related to the EXEC-dependent CF
because of the weird mixing of SALU and VALU instructions.
This change removes the code that moves REG_SEQUENCE and PHIs to VALU. Instead, we use the fact
that both REG_SEQUENCE and PHIs have copy semantics. That is, if they define SGPR but have VGPR inputs,
we insert VGPR to SGPR copies to make them pure SGPR. Then, the new copies are processed by the common
VGPR to SGPR lowering algorithm.
This is Part 2 in the series of commits aiming at the massive refactoring of the SIFixSGPRCopies pass.

Diff Detail

Event Timeline

alex-t created this revision.Jul 22 2022, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 8:15 AM
alex-t requested review of this revision.Jul 22 2022, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 8:15 AM
Herald added a subscriber: wdng. · View Herald Transcript

Overall test code changes look either positive or neutral to me.

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1034

Indention is off?

alex-t updated this revision to Diff 447992.Jul 27 2022, 3:48 AM

Fixed corner case related with AllAGPRUses in PHIs when PHI has no uses at all.
This may happen in artificial MIR tests and causes compiler error.

This change also takes care of the VGPR to SGPR copies inserted to the PHI incoming blocks.
These copies shouldbe processed out of order since the incoming blockmay have already been processed
at the moment when the PHI parent block is being processesd.

alex-t updated this revision to Diff 447994.Jul 27 2022, 3:52 AM

Formatting

alex-t marked an inline comment as done.Jul 27 2022, 3:53 AM
rampitec added inline comments.Jul 27 2022, 10:17 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1088

Indention is off?

alex-t marked an inline comment as done.Jul 27 2022, 10:44 AM
alex-t added inline comments.
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1088

Formatting is ok here.
Closing brace at line 1041 matches with 'for' at line 1006.
'if' at line 1042 has the same indentation as 'if' at line 1005 that has no braces.

rampitec added inline comments.Jul 27 2022, 10:46 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1088

This is unreadable and misleading. Add the braces then. And maybe a blank line.

rampitec added inline comments.Jul 27 2022, 11:04 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
619

It is better to clean these two at the end of the pass, to clean up memory.

alex-t updated this revision to Diff 448116.Jul 27 2022, 11:53 AM
alex-t marked an inline comment as done.

more formatting and internal maps clearing moved to the end of the pass

alex-t marked 2 inline comments as done.Jul 27 2022, 11:54 AM
This revision is now accepted and ready to land.Jul 27 2022, 12:33 PM
alex-t edited the summary of this revision. (Show Details)Jul 28 2022, 4:10 AM
foad added a comment.Jul 28 2022, 12:34 PM

As a heads-up, this patch seems to be causing some Vulkan CTS failures in LLPC testing.

alex-t reopened this revision.Aug 1 2022, 11:16 AM
This revision is now accepted and ready to land.Aug 1 2022, 11:16 AM
alex-t updated this revision to Diff 449081.Aug 1 2022, 11:16 AM

Bug caused VK CTS tests to fail fixed.

foad added inline comments.Aug 2 2022, 3:59 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
923

Don't need parens around TII->isVALU

1004–1043

Don't need the outer parens.

alex-t updated this revision to Diff 449268.Aug 2 2022, 5:27 AM

Extra parenthesis fixed

alex-t marked 2 inline comments as done.Aug 2 2022, 5:29 AM
alex-t updated this revision to Diff 449313.Aug 2 2022, 9:37 AM

rebased