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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:161-162
 
+  // Delete FeatureWavefrontSize32 functions for
+  // gfx9 and below targets that don't support the mode
+  bool IsGfx9AndBelow = ST->getGeneration() < AMDGPUSubtarget::GFX10;
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:163
+  // gfx9 and below targets that don't support the mode
+  bool IsGfx9AndBelow = ST->getGeneration() < AMDGPUSubtarget::GFX10;
+  unsigned Wave32Feature = AMDGPU::FeatureWavefrontSize32;
----------------
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)


================
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:
> 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.


================
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";
+    });
----------------
Moving this into a helper (e.g. `reportFunctionRemoved(Fn, Feature)`) would be better than duplicating the code


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

https://reviews.llvm.org/D148906