Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {}
15 changes: 15 additions & 0 deletions llvm/lib/TargetParser/TargetParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

@arsenm arsenm Aug 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@rampitec rampitec Aug 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@yxsamliu yxsamliu Aug 19, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@arsenm arsenm Aug 19, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@rampitec rampitec Aug 19, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@arsenm arsenm Aug 20, 2025

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

@rampitec rampitec Aug 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@rampitec rampitec Aug 21, 2025

Choose a reason for hiding this comment

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

#154850

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

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) {
Expand All @@ -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
Expand Down