Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AMDGPU] Fix image_msaa_load waitcnt insertion for pre-gfx12 #90710

Merged
merged 2 commits into from
May 1, 2024

Conversation

dstutt
Copy link
Collaborator

@dstutt dstutt commented May 1, 2024

#90201 made some fixes for gfx12
image_msaa_load waitcnt insertion.
That fix might break in some situations for pre-gfx12 - this fixes that by
explitly checking for VSAMPLE which always requires a s_wait_samplecnt and
leaves the previous logic intact for non-gfx12.

@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: David Stuttard (dstutt)

Changes

#90201 made some fixes for gfx12
image_msaa_load waitcnt insertion.
That fix might break in some situations for pre-gfx12 - this fixes that by
explitly checking for VSAMPLE which always requires a s_wait_samplecnt and
leaves the previous logic intact for non-gfx12.


Full diff: https://github.com/llvm/llvm-project/pull/90710.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp (+6-6)
diff --git a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
index 15a1db51c6d78b..ebb5a1af7f2d17 100644
--- a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
@@ -187,12 +187,12 @@ VmemType getVmemType(const MachineInstr &Inst) {
   const AMDGPU::MIMGInfo *Info = AMDGPU::getMIMGInfo(Inst.getOpcode());
   const AMDGPU::MIMGBaseOpcodeInfo *BaseInfo =
       AMDGPU::getMIMGBaseOpcodeInfo(Info->BaseOpcode);
-  // The test for MSAA here is because gfx12+ image_msaa_load is actually
-  // encoded as VSAMPLE and requires the appropriate s_waitcnt variant for that.
-  // Pre-gfx12 doesn't care since all vmem types result in the same s_waitcnt.
-  return BaseInfo->BVH                         ? VMEM_BVH
-         : BaseInfo->Sampler || BaseInfo->MSAA ? VMEM_SAMPLER
-                                               : VMEM_NOSAMPLER;
+  // We have to make an additional check for isVSAMPLE here since some
+  // instructions don't have a sampler, but are still classified as sampler
+  // instructions for the purposes of e.g. waitcnt.
+  return BaseInfo->BVH                                         ? VMEM_BVH
+         : (BaseInfo->Sampler || SIInstrInfo::isVSAMPLE(Inst)) ? VMEM_SAMPLER
+                                                               : VMEM_NOSAMPLER;
 }
 
 unsigned &getCounterRef(AMDGPU::Waitcnt &Wait, InstCounterType T) {

@dstutt
Copy link
Collaborator Author

dstutt commented May 1, 2024

I'm going to add a pre-commit test for this.

Fix for #90201

dstutt added 2 commits May 1, 2024 10:17
llvm#90201 made some fixes for gfx12
image_msaa_load waitcnt insertion.
That fix might break in some situations for pre-gfx12 - this fixes that by
explitly checking for VSAMPLE which always requires a s_wait_samplecnt and
leaves the previous logic intact for non-gfx12.
@dstutt
Copy link
Collaborator Author

dstutt commented May 1, 2024

See the separate commits: test, then fix + test update

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@dstutt dstutt merged commit f898161 into llvm:main May 1, 2024
3 of 4 checks passed
@dstutt dstutt deleted the gfx12-image-msaa-fixup branch May 1, 2024 10:38
jayfoad pushed a commit to jayfoad/llvm-project that referenced this pull request May 1, 2024
)

llvm#90201 made some fixes for
gfx12
image_msaa_load waitcnt insertion.
That fix might break in some situations for pre-gfx12 - this fixes that
by
explitly checking for VSAMPLE which always requires a s_wait_samplecnt
and
leaves the previous logic intact for non-gfx12.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants