This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] SIFixSGPRCopies reworking to use one pass over the MIR for analysis and lowering.
ClosedPublic

Authored by alex-t on Aug 5 2022, 1:09 AM.

Details

Summary

This change finalizes the series of patches aiming to replace the old strategy of VGPR to SGPR copy lowering.

  1. Following the https://reviews.llvm.org/D128252 and https://reviews.llvm.org/D130367 code parts that are no longer used were removed.
  2. The first pass over the MachineFunctoin collects all the necessary information.
  3. Lowering is done in 3 phases:
    • VGPR to SGPR copies analysis lowering
    • REG_SEQUENCE, PHIs, and SGPR to VGPR copies lowering
    • SCC copies lowering is done in a separate pass over the Machine Function

Diff Detail

Event Timeline

alex-t created this revision.Aug 5 2022, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 1:09 AM
alex-t requested review of this revision.Aug 5 2022, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 1:09 AM
Herald added a subscriber: wdng. · View Herald Transcript
rampitec added inline comments.Aug 5 2022, 11:35 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
646

Typo "copiy", "scoreand,hence".

773

Just MI->getParent()

1066

Just !MI->getParent()

1088

Early return here and no else.

1105

New line.

alex-t updated this revision to Diff 450580.Aug 6 2022, 4:48 PM

minor fixes as requested

alex-t marked 5 inline comments as done.Aug 6 2022, 4:50 PM
rampitec added inline comments.Aug 8 2022, 11:56 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
649

Should not it continue if tryChangeVGPRtoSGPRinCopy returned true? It will skip the next check then, which is always false.

669

Alignment is off.

alex-t updated this revision to Diff 451116.Aug 9 2022, 5:13 AM

'continue' as requested and formatting corrected

alex-t marked 2 inline comments as done.Aug 9 2022, 5:18 AM
This revision is now accepted and ready to land.Aug 9 2022, 12:30 PM
This revision was landed with ongoing or failed builds.Aug 9 2022, 3:52 PM
This revision was automatically updated to reflect the committed changes.

Reverted. Please mind your changes after commit and do not leave LLVM broken for so long.

Reverted. Please mind your changes after commit and do not leave LLVM broken for so long.

My apologies for missing the build failure notification. And thanks a lot for cleaning up the mess,

alex-t reopened this revision.Sep 13 2022, 7:53 AM
This revision is now accepted and ready to land.Sep 13 2022, 7:53 AM
alex-t requested review of this revision.Sep 13 2022, 7:54 AM
alex-t updated this revision to Diff 459750.Sep 13 2022, 8:04 AM

Fixed address sanitizer error: onDeleteListener interface added as a superclass for any client which needs the capability to handle MachineInstruction deletion in SIInstrInfo::moveToVALU.

rampitec added inline comments.Sep 13 2022, 1:05 PM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1105

New line.

1105

Why not simply clean all of these lists/vectors/maps at the end of the runOnMachineFunction?

alex-t marked an inline comment as done.Sep 14 2022, 6:17 AM
alex-t added inline comments.
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1105

The reason for that change is as follows:
We cannot lower SCC copies, REG_SEQUENCE, and PHIs before VGPR to SGPR copies since any lowering could change the MIR and, hence, make the analysis results invalid.
On other hand, in VGPR to SGPR copies lowering "SIInstrInfo::moveToVALU & Co" may be invoked. They remove MIs from the basic block and poison the memory allocated for them. So, all the pointers stored in the lists can potentially point to the poisoned memory and MI->getParent() check becomes invalid. This is the exact reason for the ASAN failure.
The "OnDeleteListener" interface serves as the delegate passed to "SIInstrInfo::moveToVALU" to make the caller informed of the MI->eraseFromParent() calls.

alex-t marked an inline comment as done.Sep 15 2022, 1:36 PM
alex-t added a reviewer: vpykhtin.

This change isn't looking as refactoring, would be nice to reflect behavior change in the description.

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

looks like accidental * removal

alex-t retitled this revision from [AMDGPU] SIFixSGPRCopies refactoring to [AMDGPU] SIFixSGPRCopies reworking to use one pass over the MIR for analysis and lowering..Sep 16 2022, 5:56 AM
alex-t edited the summary of this revision. (Show Details)
alex-t updated this revision to Diff 460713.Sep 16 2022, 6:02 AM
alex-t edited the summary of this revision. (Show Details)

accidental change removed

alex-t marked an inline comment as done.Sep 16 2022, 6:02 AM
arsenm added inline comments.Sep 16 2022, 6:17 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1105

I don't think this complicated list management is better than a separate loop

alex-t added inline comments.Sep 19 2022, 1:49 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
1105

Since we have been recently taking efforts to shrink compile time I thought that it is better.

alex-t updated this revision to Diff 461343.Sep 19 2022, 1:32 PM

MachineInstr deletion listener interface removed
SCC copies processing is done by the separate pas over the MIR

alex-t marked an inline comment as done.Sep 19 2022, 1:34 PM
rampitec added inline comments.Sep 19 2022, 1:42 PM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
647

Accidental brace move.

alex-t updated this revision to Diff 461356.Sep 19 2022, 2:09 PM

formatting

alex-t marked an inline comment as done.Sep 19 2022, 2:10 PM
rampitec accepted this revision.Sep 19 2022, 2:10 PM
This revision is now accepted and ready to land.Sep 19 2022, 2:10 PM
alex-t edited the summary of this revision. (Show Details)Sep 19 2022, 2:28 PM