[PATCH] D138141: [amdgpu] Reimplement LDS lowering

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:14
 //
-// Local variables with constant annotation or non-undef initializer are passed
+// The programming model is global variables, or equivalently function local
+// static variables, accessible from kernels or other functions. For uses from
----------------
I somehow missed this giant comment in the first pass. Perhaps this should move to AMDGPUUsage 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:161-190
+  static constexpr const char name[] = "amdgpu-lower-module-lds-strategy";
+
+  enum Opts { module, table, kernel, hybrid, unknown };
+
+  Opts Value = Opts::module;
+  operator Opts() const { return Value; }
+
----------------
You're re-inventing cl::opt with clEnumVal


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:417-418
+      if (!isKernelLDS(&F))
+        if (F.hasAddressTaken(nullptr, false, false, true /* ignore llvmused */,
+                              false)) {
+          set_union(VariablesReachableThroughFunctionPointer,
----------------
Comment the bool operand meanings 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:610
+
+      for (Use &U : llvm::make_early_inc_range(GV->uses())) {
+
----------------
Don't need llvm:: (not sure why this seems to be the most popular function for people to put it on)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:640
+        Value *replacement =
+            Builder.CreateIntToPtr(extended, GV->getType(), GV->getName());
+
----------------
Can you add an assert somewhere that the maximum LDS size fits in 16-bits? I'm also wondering if we should just make LDS pointers 16-bit with 4 byte alignment, or add a 16-bit address space for this


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:652
+
+    if (!VariableSet.empty()) {
+      for (Function &Func : M.functions()) {
----------------
Early return and reduce indentation 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:678
+
+      props() : props(nullptr, 0, 0) {}
+
----------------
Use new style member initializers 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:949-955
+        Instruction *I = dyn_cast<Instruction>(U.getUser());
+        if (!I) {
+          return false;
         }
 
-        std::string VarName =
-            (Twine("llvm.amdgcn.kernel.") + F.getName() + ".lds").str();
-        GlobalVariable *SGV;
-        DenseMap<GlobalVariable *, Constant *> LDSVarsToConstantGEP;
-        std::tie(SGV, LDSVarsToConstantGEP) =
-            createLDSVariableReplacement(M, VarName, KernelUsedVariables);
-
-        removeFromUsedLists(M, KernelUsedVariables);
-        replaceLDSVariablesWithStruct(
-            M, KernelUsedVariables, SGV, LDSVarsToConstantGEP, [&F](Use &U) {
-              Instruction *I = dyn_cast<Instruction>(U.getUser());
-              return I && I->getFunction() == &F;
-            });
-        Changed = true;
+        assert(!isKernelLDS(I->getFunction()));
+        return true;
----------------
This whole thing is just return isa<Instruction>(U.getUser())


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:995
+          OrderedKernels[i]->setMetadata("llvm.amdgcn.lds.kernel.id",
+                                         llvm::MDNode::get(Ctx, AttrMDArgs));
+
----------------
Don't need llvm::


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:171
+  // Intercept LDS variables with known addresses
+  if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(CV)) {
+    if (AMDGPUMachineFunction::isKnownAddressLDSGlobal(*GV)) {
----------------
Is this actually reachable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138141