This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GFX11] Legalize and select partial NSA MIMG instructions
ClosedPublic

Authored by mbrkusanin on Feb 14 2023, 10:37 AM.

Details

Summary

If more registers are needed for VAddr then the NSA format allows then the
final register can act as a contiguous set of remaining addresses. Update
legalizer to pack register for this new format and allow instruction
selection to use NSA encoding when number of addresses exceeds max size.
Also update SIShrinkInstructions to handle partial NSA.

Diff Detail

Event Timeline

mbrkusanin created this revision.Feb 14 2023, 10:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 10:37 AM
mbrkusanin requested review of this revision.Feb 14 2023, 10:37 AM
foad added reviewers: critson, Restricted Project.Feb 15 2023, 5:20 AM

Do we need verifier checks for the NSA restrictions on gfx11?

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
1863

Add a comment to explain there's a limit on gfx11

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4985

Can you make the descriptive of the NSA behavior rather than check/refer to gfx11+?

5001

Comment that this is packing the last register?

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
4639

Avoid generation check?

4643

This is just RI.getRegSizeInBits

Since PartialNSA is now a SubtargetFeature it can be turned off with -mattr=+partial-nsa-encoding to get old behavior.

mbrkusanin marked 4 inline comments as done.Feb 20 2023, 9:37 AM
mbrkusanin added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
1863

Using hasPartialNSAEncoding now. Should I still add a comment?

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4985

Using new subtarget feature instead.

mbrkusanin marked an inline comment as done.Feb 20 2023, 9:39 AM

Do we need verifier checks for the NSA restrictions on gfx11?

Are you referring to something other than SIInstrInfo::verifyInstruction()?

Do we need verifier checks for the NSA restrictions on gfx11?

Are you referring to something other than SIInstrInfo::verifyInstruction()?

No, that's the one I mean

Thank you for implementing this.
I have left a few minor comments inline.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6597

This condition is redundant, as you already placed the same condition in UsePartialNSA?

6669

Redundant condition as above -- presumably else branch implies UseNSA, so should not need to be a duplicate of it?

llvm/test/CodeGen/AMDGPU/cluster_stores.ll
462

It is interesting this has been able to reduce register pressure over GFX10.

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.dim.ll
1701

This doesn't need NSA.
I think you need to make some tweaks to the SIShrinkInstructions to handle partial NSA form as well.

mbrkusanin edited the summary of this revision. (Show Details)
  • Removed unnecessary if checks
  • Updated SIShrinkInstructions to handle partial NSA
mbrkusanin marked 3 inline comments as done.Feb 22 2023, 3:28 AM

Missing verifier tests

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
5050–5060

Braces

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
4640–4642

Should use getOpSize

  • new test: llvm/test/CodeGen/AMDGPU/verify-image-partial-nsa.mir
mbrkusanin marked 2 inline comments as done.Feb 22 2023, 6:23 AM
foad added inline comments.Feb 22 2023, 7:07 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6496–6497

How is this change related?

6597

I think it would be simpler overall to call getBuildDwordsVector(DAG, DL, ArrayRef(VAddrs).drop_front(NSAMaxSize - 1)) here, instead of adding another argument to getBuildDwordsVector.

6670

Can you write append_range(Ops, ArrayRef(VAddrs).take_front(NSAMaxSize - 1))?

mbrkusanin marked an inline comment as done.Feb 23 2023, 3:40 AM
mbrkusanin added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6496–6497

Sorry, mistake while rebasing with an different patch. Removed.

6597

Done. Also similar change made for GlobalISel.

6670

Done. Also similar change made for GlobalISel.

foad accepted this revision.Feb 23 2023, 4:07 AM

LGTM, thanks!

This revision is now accepted and ready to land.Feb 23 2023, 4:07 AM
This revision was landed with ongoing or failed builds.Feb 23 2023, 4:38 AM
This revision was automatically updated to reflect the committed changes.
mbrkusanin marked an inline comment as done.