[AMDGPU] Error out in clang if wavefront64 is used on gfx1250

[rampitec]

No description provided.

[rampitec]

• [AMDGPU] Error out in clang if wavefront64 is used on gfx1250 #153693 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-amdgpu

Author: Stanislav Mekhanoshin (rampitec)

Changes

---

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

2 Files Affected:

• (modified) clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl (+2)
• (modified) llvm/lib/TargetParser/TargetParser.cpp (+15)

diff --git a/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl b/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
index 4e2f7f86e8402..04de5dca3f6c0 100644
--- a/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
+++ b/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
@@ -1,8 +1,10 @@
 // RUN: not %clang_cc1 -triple amdgcn -target-feature +wavefrontsize32 -target-feature +wavefrontsize64 -o /dev/null %s 2>&1 | FileCheck %s
 // RUN: not %clang_cc1 -triple amdgcn -target-cpu gfx1103 -target-feature +wavefrontsize32 -target-feature +wavefrontsize64 -o /dev/null %s 2>&1 | FileCheck %s
 // RUN: not %clang_cc1 -triple amdgcn -target-cpu gfx900 -target-feature +wavefrontsize32 -o /dev/null %s 2>&1 | FileCheck %s --check-prefix=GFX9
+// RUN: not %clang_cc1 -triple amdgcn -target-cpu gfx1250 -target-feature +wavefrontsize64 -o /dev/null %s 2>&1 | FileCheck %s --check-prefix=GFX1250
 
 // CHECK: error: invalid feature combination: 'wavefrontsize32' and 'wavefrontsize64' are mutually exclusive
 // GFX9: error: option 'wavefrontsize32' cannot be specified on this target
+// GFX1250: error: option 'wavefrontsize64' cannot be specified on this target
 
 kernel void test() {}
diff --git a/llvm/lib/TargetParser/TargetParser.cpp b/llvm/lib/TargetParser/TargetParser.cpp
index 50b97d3257540..31558126c66b3 100644
--- a/llvm/lib/TargetParser/TargetParser.cpp
+++ b/llvm/lib/TargetParser/TargetParser.cpp
@@ -774,6 +774,18 @@ static bool isWave32Capable(StringRef GPU, const Triple &T) {
   return IsWave32Capable;
 }
 
+static bool isWave64Capable(StringRef GPU, const Triple &T) {
+  if (T.isAMDGCN()) {
+    switch (parseArchAMDGCN(GPU)) {
+    case GK_GFX1250:
+      return false;
+    default:
+      break;
+    }
+  }
+  return true;
+}
+
 std::pair<FeatureError, StringRef>
 AMDGPU::insertWaveSizeFeature(StringRef GPU, const Triple &T,
                               StringMap<bool> &Features) {
@@ -788,6 +800,9 @@ AMDGPU::insertWaveSizeFeature(StringRef GPU, const Triple &T,
   if (HaveWave32 && !IsNullGPU && !IsWave32Capable) {
     return {AMDGPU::UNSUPPORTED_TARGET_FEATURE, "wavefrontsize32"};
   }
+  if (HaveWave64 && !IsNullGPU && !isWave64Capable(GPU, T)) {
+    return {AMDGPU::UNSUPPORTED_TARGET_FEATURE, "wavefrontsize64"};
+  }
   // Don't assume any wavesize with an unknown subtarget.
   if (!IsNullGPU) {
     // Default to wave32 if available, or wave64 if not

[arsenm]

This should be added to the feature flags in the big table above, and not require multiple parse + switch functions for different cases

[rampitec]

Do you want to add a new feature to every target out there?

[yxsamliu]

I agree with Matt. We already have fillAMDGPUFeatureMap() that is supposed to fill the default features. On line 434/435, it already fills wave32/64 for spirv. It is kind of redundant and may cause inconsistency to have isWave32Capable() and isWave64Capable(). If we need a way to get wave64 capability, we could cache the default target features returned by fillAMDGPUFeatureMap() in TargetInfo and look it up for wave64 capability.

[arsenm]

Everything should be feature driven, not random GPU check driven. It's not sustainable especially when features get removed

[rampitec]

So to clarify, do you suggest adding a wavefrontsize32 or wavefrontsize64 feature in the fillAMDGPUFeatureMap to every target which can only support one wavefront size? And then if it is in the Features map assume the target cannot support another?

[arsenm]

Yes

[rampitec]

Ok, but as usual adding features to older targets will be invasive and disruptive.

[rampitec]

#154850

Turns out it is not so bad as wavefront size was inserted anyway, just at a different point.

[rampitec]

ping