[PATCH] D139000: [AMDGPU] Remove function with incompatible features

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:56
+  bool runOnModule(Module &M) override {
+    if (skipModule(M))
+      return false;
----------------
I didn't know there was a skipModule, is this the same as skipFunction in that we shouldn't be doing it for required passes?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:66
+    for (Function *F : FnsToDelete) {
+      F->replaceAllUsesWith(UndefValue::get(F->getType()));
+      F->eraseFromParent();
----------------
Should replace with ConstantPointerNull, not undef


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:76
+
+StringRef GetFeatureName(unsigned Feature) {
+  for (const SubtargetFeatureKV &KV : AMDGPUFeatureKV)
----------------
Start with lowercase


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:105
+  FeatureBitset Result = Features;
+  for (const SubtargetFeatureKV &FE : AMDGPUFeatureKV)
+    if (Features.test(FE.Value) && FE.Implies.any())
----------------
Braces


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:118
+  // This pass is primarily intended for GCN, so check we have a GCN GPU.
+  if (!TM->getTargetTriple().isAMDGCN())
+    return false;
----------------
Don't need to do this per function, can just not run the pass in the first place


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:153-155
+      std::string Msg =
+          "+" + GetFeatureName(Feature).str() +
+          " is not supported on the current target. Removing function.";
----------------
You can build this twine directly top ass to DiagnosticInfo, you don't need to build a std::string first


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURemoveIncompatibleFunctions.cpp:156
+          " is not supported on the current target. Removing function.";
+      DiagnosticInfoUnsupported DiagInfo(F, Msg, DiagnosticLocation(),
+                                         DS_Warning);
----------------
I'm not sure this really belongs under DiagnosticInfoUnsupported, seems like its own category (also, it's not a warning if it's supposed to happen)


================
Comment at: llvm/test/CodeGen/AMDGPU/remove-incompatible-functions.ll:293
+if:
+  %1 = load i64, i64 addrspace(1)* %in
+  br label %endif
----------------
Use opaque pointers and named values 


================
Comment at: llvm/test/CodeGen/AMDGPU/remove-incompatible-functions.ll:845
+; GFX11:       ; %bb.0:
+; GFX11-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GFX11-NEXT:    s_waitcnt_vscnt null, 0x0
----------------
Also should include IR check lines


================
Comment at: llvm/test/CodeGen/AMDGPU/remove-incompatible-functions.ll:856
+}
+
+attributes #0 = { "target-features"="+dpp" }
----------------
Missing uses for the functions? In addition to some direct callers, I would like to see some address captures and constantexpr uses


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139000