RFC: [AMDGPU] Check subtarget features for consistency #86957

jayfoad · 2024-03-28T14:42:06Z

Implement GCNSubtarget::checkSubtargetFeatures as a canonical place to
check subtarget features for consistency and diagnose any
inconsistencies. To start with, the implementation just checks that
either wavefrontsize32 or wavefrontsize64 is selected.

checkSubtargetFeatures is called at the start of instruction selection.
This is pretty arbitrary. It is just a convenient point at which we have
access to the subtarget that we're going to use for codegenning a
particular function.

Implement GCNSubtarget::checkSubtargetFeatures as a canonical place to check subtarget features for consistency and diagnose any inconsistencies. To start with, the implementation just checks that either wavefrontsize32 or wavefrontsize64 is selected. checkSubtargetFeatures is called at the start of instruction selection. This is pretty arbitrary. It is just a convenient point at which we have access to the subtarget that we're going to use for codegenning a particular function.

llvmbot · 2024-03-28T14:42:35Z

@llvm/pr-subscribers-backend-amdgpu

Author: Jay Foad (jayfoad)

Changes

Implement GCNSubtarget::checkSubtargetFeatures as a canonical place to
check subtarget features for consistency and diagnose any
inconsistencies. To start with, the implementation just checks that
either wavefrontsize32 or wavefrontsize64 is selected.

checkSubtargetFeatures is called at the start of instruction selection.
This is pretty arbitrary. It is just a convenient point at which we have
access to the subtarget that we're going to use for codegenning a
particular function.

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

6 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp (+1)
(modified) llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp (+1)
(modified) llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp (+9)
(modified) llvm/lib/Target/AMDGPU/GCNSubtarget.h (+4)
(added) llvm/test/CodeGen/AMDGPU/check-subtarget-features.ll (+10)
(modified) llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll (+4-4)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp b/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
index bba7682cd7a0d1..c11c7a57e05966 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
@@ -132,6 +132,7 @@ bool AMDGPUDAGToDAGISel::runOnMachineFunction(MachineFunction &MF) {
   }
 #endif
   Subtarget = &MF.getSubtarget<GCNSubtarget>();
+  Subtarget->checkSubtargetFeatures(MF.getFunction());
   Mode = SIModeRegisterDefaults(MF.getFunction(), *Subtarget);
   return SelectionDAGISel::runOnMachineFunction(MF);
 }
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp b/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
index e13c13913d4e82..b48a09489653a1 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
@@ -63,6 +63,7 @@ void AMDGPUInstructionSelector::setupMF(MachineFunction &MF, GISelKnownBits *KB,
                                         BlockFrequencyInfo *BFI) {
   MRI = &MF.getRegInfo();
   Subtarget = &MF.getSubtarget<GCNSubtarget>();
+  Subtarget->checkSubtargetFeatures(MF.getFunction());
   InstructionSelector::setupMF(MF, KB, CoverageInfo, PSI, BFI);
 }
 
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
index fa77b94fc22def..fce72ed504d445 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
@@ -25,6 +25,7 @@
 #include "llvm/CodeGen/GlobalISel/InlineAsmLowering.h"
 #include "llvm/CodeGen/MachineScheduler.h"
 #include "llvm/CodeGen/TargetFrameLowering.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
 #include "llvm/IR/IntrinsicsR600.h"
 #include "llvm/IR/MDBuilder.h"
@@ -165,6 +166,14 @@ GCNSubtarget::initializeSubtargetDependencies(const Triple &TT,
   return *this;
 }
 
+void GCNSubtarget::checkSubtargetFeatures(const Function &F) const {
+  if (hasFeature(AMDGPU::FeatureWavefrontSize32) ==
+      hasFeature(AMDGPU::FeatureWavefrontSize64)) {
+    F.getContext().diagnose(DiagnosticInfoUnsupported(
+        F, "must specify exactly one of wavefrontsize32 and wavefrontsize64"));
+  }
+}
+
 AMDGPUSubtarget::AMDGPUSubtarget(const Triple &TT) : TargetTriple(TT) {}
 
 bool AMDGPUSubtarget::useRealTrue16Insts() const {
diff --git a/llvm/lib/Target/AMDGPU/GCNSubtarget.h b/llvm/lib/Target/AMDGPU/GCNSubtarget.h
index 4da10beabe3162..da87c6852f1969 100644
--- a/llvm/lib/Target/AMDGPU/GCNSubtarget.h
+++ b/llvm/lib/Target/AMDGPU/GCNSubtarget.h
@@ -248,6 +248,10 @@ class GCNSubtarget final : public AMDGPUGenSubtargetInfo,
   GCNSubtarget &initializeSubtargetDependencies(const Triple &TT,
                                                    StringRef GPU, StringRef FS);
 
+  /// Diagnose inconsistent subtarget features before attempting to codegen
+  /// function \p F.
+  void checkSubtargetFeatures(const Function &F) const;
+
   const SIInstrInfo *getInstrInfo() const override {
     return &InstrInfo;
   }
diff --git a/llvm/test/CodeGen/AMDGPU/check-subtarget-features.ll b/llvm/test/CodeGen/AMDGPU/check-subtarget-features.ll
new file mode 100644
index 00000000000000..c2469398110466
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/check-subtarget-features.ll
@@ -0,0 +1,10 @@
+; RUN: not llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1100 -mattr=-wavefrontsize32,-wavefrontsize64 < %s 2>&1 | FileCheck %s -check-prefix=ERR -implicit-check-not=error:
+; RUN: not llc -global-isel=1 -mtriple=amdgcn -mcpu=gfx1100 -mattr=-wavefrontsize32,-wavefrontsize64 < %s 2>&1 | FileCheck %s -check-prefix=ERR -implicit-check-not=error:
+; RUN: not llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1100 -mattr=+wavefrontsize32,+wavefrontsize64 < %s 2>&1 | FileCheck %s -check-prefix=ERR -implicit-check-not=error:
+; RUN: not llc -global-isel=1 -mtriple=amdgcn -mcpu=gfx1100 -mattr=+wavefrontsize32,+wavefrontsize64 < %s 2>&1 | FileCheck %s -check-prefix=ERR -implicit-check-not=error:
+
+; ERR: error: {{.*}} in function f void (): must specify exactly one of wavefrontsize32 and wavefrontsize64
+
+define void @f() {
+  ret void
+}
diff --git a/llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll b/llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll
index 8ef1d3ff27e51d..406c953a06d974 100644
--- a/llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll
+++ b/llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll
@@ -8,13 +8,13 @@
 ; RUN: FileCheck --check-prefix=WARN-GFX90A %s < %t
 ; RUN: llc -mtriple=amdgcn -mcpu=gfx90a -mattr=+wavefrontsize64 -verify-machineinstrs < %s
 
-; RUN: llc -mtriple=amdgcn -mcpu=gfx1011 -mattr=+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
+; RUN: llc -mtriple=amdgcn -mcpu=gfx1011 -mattr=-wavefrontsize32,+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
 ; RUN:   -pass-remarks=amdgpu-remove-incompatible-functions < %s 2>%t | FileCheck -check-prefixes=GFX10 %s
-; RUN: llc -mtriple=amdgcn -mcpu=gfx1011 -mattr=+wavefrontsize64 -verify-machineinstrs < %s
+; RUN: llc -mtriple=amdgcn -mcpu=gfx1011 -mattr=-wavefrontsize32,+wavefrontsize64 -verify-machineinstrs < %s
 
-; RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -mattr=+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
+; RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -mattr=-wavefrontsize32,+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
 ; RUN:   -pass-remarks=amdgpu-remove-incompatible-functions < %s 2>%t | FileCheck -check-prefixes=GFX11 %s
-; RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -mattr=+wavefrontsize64 -verify-machineinstrs < %s
+; RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -mattr=-wavefrontsize32,+wavefrontsize64 -verify-machineinstrs < %s
 
 ; WARN-GFX906: removing function 'needs_wavefrontsize32': +wavefrontsize32 is not supported on the current target
 ; WARN-GFX906-NOT: not supported

jayfoad · 2024-03-28T14:43:33Z

TODO: CodeGen/AMDGPU/unknown-processor.ll fails because it doesn't set any subtarget features. I don't know how this should be handled. Do we really need to support -mcpu=unknown???

arsenm

I think we need a target dependent IR verifier, but have never been sure where to put such a thing

arsenm · 2024-03-28T17:40:29Z

TODO: CodeGen/AMDGPU/unknown-processor.ll fails because it doesn't set any subtarget features. I don't know how this should be handled. Do we really need to support -mcpu=unknown???

We need to support the none case for library builds, but I've been debating rejecting codegenning it. It's a nonstop source of bug reports where the default of "almost Tahiti" fails to select flat pointer load/store

Pierre-vh · 2024-04-16T07:23:11Z

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp

+void GCNSubtarget::checkSubtargetFeatures(const Function &F) const {
+  if (hasFeature(AMDGPU::FeatureWavefrontSize32) ==
+      hasFeature(AMDGPU::FeatureWavefrontSize64)) {
+    F.getContext().diagnose(DiagnosticInfoUnsupported(


tiny tiny nit: put F.getContext() in a variable?

arsenm

Don't think this is the ideal place, but it's better than nothing

jayfoad · 2024-05-08T16:33:56Z

TODO: CodeGen/AMDGPU/unknown-processor.ll fails because it doesn't set any subtarget features. I don't know how this should be handled. Do we really need to support -mcpu=unknown???

We need to support the none case for library builds, but I've been debating rejecting codegenning it. It's a nonstop source of bug reports where the default of "almost Tahiti" fails to select flat pointer load/store

Any suggestions what to do about this for now, so that I can land this patch without breaking it?

arsenm · 2024-05-08T16:39:42Z

TODO: CodeGen/AMDGPU/unknown-processor.ll fails because it doesn't set any subtarget features. I don't know how this should be handled. Do we really need to support -mcpu=unknown???

We need to support the none case for library builds, but I've been debating rejecting codegenning it. It's a nonstop source of bug reports where the default of "almost Tahiti" fails to select flat pointer load/store

Any suggestions what to do about this for now, so that I can land this patch without breaking it?

I've been leaning towards disallowing codegen for the none target. We get a steady stream of bug reports from people trying to apply it to random IR, but this is probably a more disruptive and time consuming change.

Also, I've been leaning towards moving the wavesize out of the subtarget features. It's really more of an ABI trait that would be better off as either a family of calling conventions, or a separate function attribute .

For the purpose of this change, it's probably OK to ignore it. In practice the default target is just wave64

jayfoad · 2024-05-09T09:39:33Z

For the purpose of this change, it's probably OK to ignore it.

Like this? I just added not to the failing RUN line.

arsenm · 2024-05-09T09:45:43Z

For the purpose of this change, it's probably OK to ignore it.

Like this? I just added not to the failing RUN line.

I think the current "unknown processor" behavior of warn and proceed is utter nonsense. However, this is a different case to codegening the none/default/generic processor

llvmbot added the backend:AMDGPU label Mar 28, 2024

jayfoad requested review from arsenm and Pierre-vh March 28, 2024 14:42

arsenm reviewed Mar 28, 2024

View reviewed changes

Pierre-vh approved these changes Apr 16, 2024

View reviewed changes

Put context in a variable

8e81e28

arsenm mentioned this pull request Apr 25, 2024

target amdgcn-amd-amdhsa: cttz in BB after amdgcn.ballot leads to "Cannot select SETCC..." #89332

Closed

arsenm approved these changes May 8, 2024

View reviewed changes

Allow -mtriple=amdgcn -mcpu=unknown to fail

ccbeb98

jayfoad added 2 commits May 9, 2024 11:34

Merge remote-tracking branch 'origin/main' into check-subtarget-features

134c44f

Merge remote-tracking branch 'origin/main' into check-subtarget-features

7233d02

jayfoad merged commit 6eb9e21 into llvm:main May 9, 2024

jayfoad deleted the check-subtarget-features branch May 9, 2024 10:37

This was referenced Jun 21, 2024

[AMDGPU] Introduce a pseudo mnemonic for S_DELAY_ALU in MIR. #96004

Merged

[AMDGPU] Fix MC/Disassembler/AMDGPU/decode-err.txt. #96621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RFC: [AMDGPU] Check subtarget features for consistency #86957

RFC: [AMDGPU] Check subtarget features for consistency #86957

Uh oh!

jayfoad commented Mar 28, 2024

Uh oh!

llvmbot commented Mar 28, 2024

Uh oh!

jayfoad commented Mar 28, 2024

Uh oh!

arsenm left a comment

Uh oh!

arsenm commented Mar 28, 2024

Uh oh!

Pierre-vh Apr 16, 2024

Uh oh!

arsenm left a comment

Uh oh!

jayfoad commented May 8, 2024

Uh oh!

arsenm commented May 8, 2024

Uh oh!

jayfoad commented May 9, 2024

Uh oh!

arsenm commented May 9, 2024

Uh oh!

Uh oh!

RFC: [AMDGPU] Check subtarget features for consistency #86957

RFC: [AMDGPU] Check subtarget features for consistency #86957

Uh oh!

Conversation

jayfoad commented Mar 28, 2024

Uh oh!

llvmbot commented Mar 28, 2024

Uh oh!

jayfoad commented Mar 28, 2024

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm commented Mar 28, 2024

Uh oh!

Pierre-vh Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

jayfoad commented May 8, 2024

Uh oh!

arsenm commented May 8, 2024

Uh oh!

jayfoad commented May 9, 2024

Uh oh!

arsenm commented May 9, 2024

Uh oh!

Uh oh!