[AMDGPU] Remove function if FeatureWavefrontSize 32 is not supported on current GPU

[skc7]

Summary

Currently, wavfrontsize32 is being appended by device-libs to some functions for gfx9 targets.
For gfx9 and below targets, this change deletes the incompatible function, since FeatureWavefrontSize32 is not supported.

[foad]

Currently, wavfrontsize32 is being appended by device-libs to some functions for gfx9 targets.

Why? That seems wrong.

[arsenm]

In D148906#4286691, @foad wrote:

Currently, wavfrontsize32 is being appended by device-libs to some functions for gfx9 targets.

Why? That seems wrong.

It is wrong but that’s how the library is currently structured. All code coexists in the same IR, and there are some wave32 only functions

[yaxunl]

In D148906#4286903, @arsenm wrote:

In D148906#4286691, @foad wrote:

Currently, wavfrontsize32 is being appended by device-libs to some functions for gfx9 targets.

Why? That seems wrong.

It is wrong but that’s how the library is currently structured. All code coexists in the same IR, and there are some wave32 only functions

One concern is whether removing +wavefrontsize32 will make them work. They may contain an instruction sequence that only works with wavefront size 32.

[JonChesterfield]

If device libs doesn't want to change that, we could provide an attribute or similar that device libs annotates functions with. Wave32-only or similar, with semantics that it gets erased by clang without warning on wave64. Would be as if that function never existed on wave64 targets. Might need it to apply to objects as well, depending on how much of this stuff device-libs contains.

[foad]

In D148906#4286903, @arsenm wrote:

In D148906#4286691, @foad wrote:

Currently, wavfrontsize32 is being appended by device-libs to some functions for gfx9 targets.

Why? That seems wrong.

It is wrong but that’s how the library is currently structured. All code coexists in the same IR, and there are some wave32 only functions

But if you're compiling for gfx900 then this pass should remove any functions that are marked with +wavefrontsize32. It should not just remove the +wavefrontsize32 from the function.

[b-sumner]

In D148906#4287110, @foad wrote:

In D148906#4286903, @arsenm wrote:

In D148906#4286691, @foad wrote:

Currently, wavfrontsize32 is being appended by device-libs to some functions for gfx9 targets.

Why? That seems wrong.

It is wrong but that’s how the library is currently structured. All code coexists in the same IR, and there are some wave32 only functions

But if you're compiling for gfx900 then this pass should remove any functions that are marked with +wavefrontsize32. It should not just remove the +wavefrontsize32 from the function.

I thought we had agreed that we were going to eliminate the body of incompatible functions and that had been implemented. If that is not enough, then maybe we should strip the problematic attributes of such functions as well?

[foad]

It looks like you just want a one-liner patch that adds FeatureWavefrontSize32 to the FeaturesToCheck table?

[yaxunl]

In D148906#4286979, @JonChesterfield wrote:

If device libs doesn't want to change that, we could provide an attribute or similar that device libs annotates functions with. Wave32-only or similar, with semantics that it gets erased by clang without warning on wave64. Would be as if that function never existed on wave64 targets. Might need it to apply to objects as well, depending on how much of this stuff device-libs contains.

device libs may be linked in by other programs or libraries, e.g. comgr.

[JonChesterfield]

In D148906#4287243, @yaxunl wrote:

device libs may be linked in by other programs or libraries, e.g. comgr.

That used to be broken. Clang mlink-builtin-bitcode applies attributes to functions in device libs that were both required and missing from device libs. Caused a bunch of pain in openmp because we really wanted to link device libs at link time, not into every TU up front in clang. Has someone fixed that requirement?

[Pierre-vh]

In D148906#4287237, @foad wrote:

It looks like you just want a one-liner patch that adds FeatureWavefrontSize32 to the FeaturesToCheck table?

Indeed, if your goal is just to remove the functions that have +wavefrontsize32 on targets that doesn't support it, this is all you need to do.

I'm also not a fan of removing attributes in this pass. The pass is documented as "removes any function with incompatible attributes". If it also starts removing attributes then its description (& the CL option) would need to be updated to reflect the change (but really, I think this should be done somewhere else, perhaps in yet another pass).

[yaxunl]

In D148906#4289053, @JonChesterfield wrote:

In D148906#4287243, @yaxunl wrote:

device libs may be linked in by other programs or libraries, e.g. comgr.

That used to be broken. Clang mlink-builtin-bitcode applies attributes to functions in device libs that were both required and missing from device libs. Caused a bunch of pain in openmp because we really wanted to link device libs at link time, not into every TU up front in clang. Has someone fixed that requirement?

OpenCL uses AMDGPUSimplifyLibCalls with -amdgpu-prelink, which transforms some calls of device lib functions into calls of new device lib functions, e.g. sincos. Since this happens after clang's -mlink-builtin-bitcode, these newly introduced device lib functions need to be linked in by comgr directly.

But that is not the only situation that device libs are linked by non-clang programs. There are other FE's that generate IR for amdgpu and need link with device libs, e.g. XLA.

That said, I think the question is that whether simply removing +wavefrontsize32 makes a function valid for wave64. There must be a reason that a function in device lib is marked with +wavefrontsize32. I assume that a function marked with +wavefrontsize32 means it can only be used with wave32. If that is true, then such functions should just be treated as incompatible functions and treated the same way.

[b-sumner]

I think a function with wavefrontsize32 is incompatible with a wavefrontsize64 compilation and similarly for a wavefrontsize64 function with a wavefrontsize32 compilation and we should take the usual steps for incompatible functions.

[skc7]

Updated patch to delete the function if FeatureWavefrontSize32 is not supported by current GPU.

[arsenm]

I'd need to think harder about whether it's ok to simply remove functions with wavefrontsize32 if you aren't compiling for wave32. It's not quite like the other features, as it behaves more like a selectable mode. I would expect we delete wave32 functions for <gfx10 targets that don't support the mode

llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll

27

-NOT checks are fragile and should be as loose as possible. Drop the "define void" and trailing (

[b-sumner]

I think we need to similarly remove a wavefrontsize64 function if compiling in wave32 mode.

[skc7]

Remove wavefrontsize64 function if compiling in wave32 mode.

[skc7]

llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp

160

@arsenm AFAIU, This condition checks that (for ex.) if wave32 mode is enabled and current GPU doesn't support waveforntsize32 feature, then that function should be deleted.
This inherently fulfills the condition "delete wave32 functions for <gfx10 targets that don't support the mode" ? Please let me know if I'm missing anything here.

[skc7]

expandImpliedFeatures gets all the features implied by current GPU. But wavefrontsize32 and 64 are not part of the feature list for gfx10 and above targets in AMDGPU.td. AFAIU, It is assumed those subtarget supports both the features if they are not part of the list. So GPUFeatureBits cannot be relied on to query FeatureWavefrontSize32/64.

In the latest revision, made changes to delete FeatureWavefrontSize32 functions for gfx9 and below targets that don't support the mode

[Pierre-vh]

llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp

166–167

Can you explain in the comment why we need to check this separately?
Just saying "GFX10+ is implied to support both wave32 and 64, they are not in the feature set, so we need a separate check" is enough

168

IMO there is no need to make a separate bool, just inline it in the if, but no strong preference here (though make the bool const if you keep it)

168–169

IMO the first variable isn't really needed, just inline the check in the if. The second one could also just be inlined because the feature won't change, but no strong preference here.

If you keep either/both of them, make them const though.

171–177

Moving this into a helper (e.g. reportFunctionRemoved(Fn, Feature)) would be better than duplicating the code

[foad]

I think we need to similarly remove a wavefrontsize64 function if compiling in wave32 mode.

I think this is a reasonable request but it's an extension to what this pass currently does.

• Currently the pass removes functions that rely on features not supported by the -mcpu= GPU.
• The extension would be to remove functions that rely on features not supported by the -mcpu= GPU as modified by any -mattr= options.

Does that make sense? If so can we please implement that in a generic way instead of having a special hack for wave32/wave4 features?

[skc7]

Updated patch as per feedback from @Pierre-vh

[b-sumner]

In D148906#4321445, @foad wrote:

I think we need to similarly remove a wavefrontsize64 function if compiling in wave32 mode.

I think this is a reasonable request but it's an extension to what this pass currently does.

• Currently the pass removes functions that rely on features not supported by the -mcpu= GPU.
• The extension would be to remove functions that rely on features not supported by the -mcpu= GPU as modified by any -mattr= options.

Does that make sense? If so can we please implement that in a generic way instead of having a special hack for wave32/wave4 features?

It makes sense to me. The library doesn't need it currently, but there has been a use case to detect CUmode in the headers, and that could arrive in the library where it would be useful to rely on either the CUmode or WGPmode version of a function to be removed depending on the mode.

[skc7]

Rebase.

[skc7]

llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp

168–169

Updated the comments. Thanks for feedback.

171–177

Moved it to reportFunctionRemoved. Thanks for feedback.

llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll

27

Fixed it. Drop the "define void" and trailing (

[arsenm]

llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll

31–33

Note these functions don't really "require" wave 32. They will codegen just fine without it

llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp

 FeatureBitset expandImpliedFeatures(const FeatureBitset &Features) {
   FeatureBitset Result = Features;
   for (const SubtargetFeatureKV &FE : AMDGPUFeatureKV) {
     if (Features.test(FE.Value) && FE.Implies.any())
       Result |= expandImpliedFeatures(FE.Implies.getAsBitset());
   }
   return Result;
 }
+
+void reportFunctionRemoved(Function &F, unsigned Feature) {
+  OptimizationRemarkEmitter ORE(&F);
+  ORE.emit([&]() {
+    // Note: we print the function name as part of the diagnostic because if
+    // debug info is not present, users get "<unknown>:0:0" as the debug
+    // loc. If we didn't print the function name there would be no way to
+    // tell which function got removed.
+    return OptimizationRemark(DEBUG_TYPE, "AMDGPUIncompatibleFnRemoved", &F)
+           << "removing function '" << F.getName() << "': +"
+           << getFeatureName(Feature)
+           << " is not supported on the current target";
+  });
+  return;
+}
 } // end anonymous namespace
 
 bool AMDGPURemoveIncompatibleFunctions::checkFunction(Function &F) {
   if (F.isDeclaration())
     return false;
 
   const GCNSubtarget *ST =
       static_cast<const GCNSubtarget *>(TM->getSubtargetImpl(F));
const FeatureBitset GPUFeatureBits =
       expandImpliedFeatures(GPUInfo->Implies.getAsBitset());
 
   // Now that the have a FeatureBitset containing all possible features for
   // the chosen GPU, check our list of "suspicious" features.
 
   // Check that the user didn't enable any features that aren't part of that
   // GPU's feature set. We only check a predetermined set of features.
   for (unsigned Feature : FeaturesToCheck) {
     if (ST->hasFeature(Feature) && !GPUFeatureBits.test(Feature)) {
-      OptimizationRemarkEmitter ORE(&F);
-      ORE.emit([&]() {
-        // Note: we print the function name as part of the diagnostic because if
-        // debug info is not present, users get "<unknown>:0:0" as the debug
-        // loc. If we didn't print the function name there would be no way to
-        // tell which function got removed.
-        return OptimizationRemark(DEBUG_TYPE, "AMDGPUIncompatibleFnRemoved", &F)
-               << "removing function '" << F.getName() << "': +"
-               << getFeatureName(Feature)
-               << " is not supported on the current target";
-      });
+      reportFunctionRemoved(F, Feature);
       return true;
     }
   }
 
+  // Delete FeatureWavefrontSize32 functions for
+  // gfx9 and below targets that don't support the mode.
+  // gfx10+ is implied to support both wave32 and 64 features.
+  // They are not in the feature set. So, we need a separate check
+  if (ST->getGeneration() < AMDGPUSubtarget::GFX10 &&
+      ST->hasFeature(AMDGPU::FeatureWavefrontSize32)) {
+    reportFunctionRemoved(F, AMDGPU::FeatureWavefrontSize32);
+    return true;
+  }
   return false;
 }
 
 INITIALIZE_PASS(AMDGPURemoveIncompatibleFunctions, DEBUG_TYPE,
                 "AMDGPU Remove Incompatible Functions", false, false)
 
 char AMDGPURemoveIncompatibleFunctions::ID = 0;
 
 ModulePass *
 llvm::createAMDGPURemoveIncompatibleFunctionsPass(const TargetMachine *TM) {
   return new AMDGPURemoveIncompatibleFunctions(TM);
 }

llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll

+; RUN: llc -march=amdgcn -mcpu=gfx906 -mattr=+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
+; RUN:   -pass-remarks=amdgpu-remove-incompatible-functions < %s 2>%t | FileCheck -check-prefixes=GFX906 %s
+; RUN: FileCheck --check-prefix=WARN-GFX906 %s < %t
+; RUN: llc -march=amdgcn -mcpu=gfx906 -mattr=+wavefrontsize64 -verify-machineinstrs < %s
+
+; RUN: llc -march=amdgcn -mcpu=gfx90a -mattr=+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
+; RUN:   -pass-remarks=amdgpu-remove-incompatible-functions < %s 2>%t | FileCheck -check-prefixes=GFX90A %s
+; RUN: FileCheck --check-prefix=WARN-GFX90A %s < %t
+; RUN: llc -march=amdgcn -mcpu=gfx90a -mattr=+wavefrontsize64 -verify-machineinstrs < %s
+
+; RUN: llc -march=amdgcn -mcpu=gfx1011 -mattr=+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
+; RUN:   -pass-remarks=amdgpu-remove-incompatible-functions < %s 2>%t | FileCheck -check-prefixes=GFX10 %s
+; RUN: llc -march=amdgcn -mcpu=gfx1011 -mattr=+wavefrontsize64 -verify-machineinstrs < %s
+
+; RUN: llc -march=amdgcn -mcpu=gfx1100 -mattr=+wavefrontsize64 -stop-after=amdgpu-remove-incompatible-functions\
+; RUN:   -pass-remarks=amdgpu-remove-incompatible-functions < %s 2>%t | FileCheck -check-prefixes=GFX11 %s
+; RUN: llc -march=amdgcn -mcpu=gfx1100 -mattr=+wavefrontsize64 -verify-machineinstrs < %s
+
+; WARN-GFX906: removing function 'needs_wavefrontsize32': +wavefrontsize32 is not supported on the current target
+; WARN-GFX906-NOT: not supported
+
+; WARN-GFX90A: removing function 'needs_wavefrontsize32': +wavefrontsize32 is not supported on the current target
+; WARN-GFX90A-NOT: not supported
+
+define void @needs_wavefrontsize32(ptr %out) #0 {
+; GFX906-NOT:   @needs_wavefrontsize32
+; GFX90A-NOT:   @needs_wavefrontsize32
+; GFX10:        define void @needs_wavefrontsize32(
+; GFX11:        define void @needs_wavefrontsize32(
+  %1 = tail call i32 @llvm.read_register.i32(metadata !0)
+  %2 = tail call i32 @llvm.ctpop.i32(i32 %1)
+  store i32 %2, ptr %out, align 4
+  ret void
+}
+
+define void @caller(ptr %out) {
+  ; GFX906: call void null(
+  ; GFX90A: call void null(
+  ; GFX10: call void @needs_wavefrontsize32(
+  ; GFX11: call void @needs_wavefrontsize32(
+  call void @needs_wavefrontsize32(ptr %out)
+  ret void
+}
+
+declare i32 @llvm.read_register.i32(metadata)
+declare i32 @llvm.ctpop.i32(i32)
+
+!0 = !{!"exec_lo"}
+
+attributes #0 = { "target-features"="+wavefrontsize32" }