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

[krishna chaitanya sankisa via Phabricator via llvm-commits]

skc7 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:163-164
+  // gfx9 and below targets that don't support the mode
+  bool IsGfx9AndBelow = ST->getGeneration() < AMDGPUSubtarget::GFX10;
+  unsigned Wave32Feature = AMDGPU::FeatureWavefrontSize32;
+  if (IsGfx9AndBelow && ST->hasFeature(Wave32Feature)) {
----------------
Pierre-vh wrote:
> Pierre-vh wrote:
> > 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)
> 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.
Updated the comments. Thanks for feedback.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:166-172
+    OptimizationRemarkEmitter ORE(&F);
+    ORE.emit([&]() {
+      return OptimizationRemark(DEBUG_TYPE, "AMDGPUIncompatibleFnRemoved", &F)
+             << "removing function '" << F.getName() << "': +"
+             << getFeatureName(Wave32Feature)
+             << " is not supported on the current target";
+    });
----------------
Pierre-vh wrote:
> Moving this into a helper (e.g. `reportFunctionRemoved(Fn, Feature)`) would be better than duplicating the code
Moved it to reportFunctionRemoved. Thanks for feedback.


================
Comment at: llvm/test/CodeGen/AMDGPU/remove-incompatible-wave32-feature.ll:26
+define void @needs_wavefrontsize32(ptr %out) #0 {
+; GFX906-NOT:   define void @needs_wavefrontsize32(
+; GFX90A-NOT:   define void @needs_wavefrontsize32(
----------------
arsenm wrote:
> -NOT checks are fragile and should be as loose as possible. Drop the "define void" and trailing (
Fixed it. Drop the "define void" and trailing (


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148906/new/

https://reviews.llvm.org/D148906