[PATCH] D91516: [AMDGPU][WIP] Lower Function Local LDS Variables.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

All tests are now missing



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:32
+
+static bool isKernel(Function *F) {
+  if (AMDGPU::isModuleEntryFunctionCC(F->getCallingConv()))
----------------
This function is pointless, just directly use isModuleEntryFunctionCC


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:41-44
+static void assertCallGraphNodePtrIsNonNull(CallGraphNode *CGN) {
+  assert(CGN && "Call graph node associated with kernel/function definition "
+                "cannot be null.\n");
+}
----------------
This function is useless. Assert strings also don't need to end in \n


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:56
+    assertCallGraphNodePtrIsNonNull(CGN);
+    assert(CB && "Call base associated with a call site within call graph "
+                 "cannot be null\n");
----------------
cast<>, don't dyn_cast and assert


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:66
+class LowerFunctionLocalLDSImpl {
+  // Object constructors and entry-point functions.
+public:
----------------
Pointless comment


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:68
+public:
+  // Constructs a LowerFunctionLocalLDSImpl object for the given Module.
+  explicit LowerFunctionLocalLDSImpl(Module &M) : M(M), CG(CallGraph(M)) {}
----------------
Pointless comment


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:74-75
+
+  // Internal data structures which are being constructed while constructing
+  // the class object itself.
+private:
----------------
Pointless comment


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:82
+  // `initialize()` after the class object is constructed.
+private:
+  SmallPtrSet<Function *, 16> Kernels;
----------------
Extra private


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:92
+  // Helper private member functions.
+private:
+  // Filter out unhanlded kernels. Unhandled kernels are those which do not have
----------------
Extra private


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:93
+private:
+  // Filter out unhanlded kernels. Unhandled kernels are those which do not have
+  // any function local LDS to be lowered w.r.t them.
----------------
Typo unhanlded


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:111
+    for (auto *LDS : LDSGlobals)
+      if (LDSToFunction.find(LDS) == LDSToFunction.end())
+        ToBeRemovedLDSList.insert(LDS);
----------------
.contains


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:142
+  // Private member functions which build different data structures.
+private:
+  // Associate current kernel K with LDS set which are supposed to be lowered
----------------
Extra private


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:233
+      continue;
+    std::set<GlobalVariable *> CalleeLDSSet = FunctionToLDS[Callee];
+    for (auto *CalleeLDS : CalleeLDSSet)
----------------
Copy of set unnecessary


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:320
+      auto *F = CGN->getFunction();
+      assert(F && "Expected a valid address taken function.\n");
+      auto *ADFTy = F->getFunctionType();
----------------
Don't need all these newlines in assert strings


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:376-379
+  // Push the `CallGraphNode` associated with all the callees of the kernel`K`
+  // into into `CGNodeStack`, and the corresponding call sites into
+  // `CallBaseStack`.
+  pushCallGraphNodes(KernCGNode, CGNodeStack, CallBaseStack);
----------------
I think you're overcomplicating the CallGraph usage by ignoring most of what it gives you. You should be able to just iterate directly through the CallGraph to get functions reachable from the parent


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:410-411
+
+// Create a reverse map from function to LDS set which maps a given function F
+// to a set of LDS which are defined within F.
+void LowerFunctionLocalLDSImpl::createFunctionToLDSMap() {
----------------
This concept doesn't quite work for the IR. The same global can appear in multiple functions


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:449
+    if (!I) {
+      assert(dyn_cast<Constant>(U) && "Expected a constant expression\n.");
+      append_range(UserStack, U->users());
----------------
isa<>, no \n


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:479
+    // FIXME: Anything else need to be excluded?
+    if (!F || F->getName().startswith("llvm.amdgcn.") || isKernel(F))
+      continue;
----------------
Should not be checking the function name. Should just skip all declarations


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:492-495
+  if (Kernels.empty())
+    return false;
+
+  return true;
----------------
Return !Kernels.empty()


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:502
+    if (GV.getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS &&
+        !GV.hasExternalLinkage())
+      LDSGlobals.insert(&GV);
----------------
Probably should skip declarations. Also not sure about the linkage check


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:506-509
+  if (LDSGlobals.empty())
+    return false;
+
+  return true;
----------------
Return !empty()


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:555-558
+  if (Kernels.empty())
+    return false;
+
+  return true;
----------------
Return !empty()


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLocalLDS.cpp:561-571
+// Entry-point function.
+bool LowerFunctionLocalLDSImpl::lower() {
+  // Build necessary data structures which will be later used in the lowering
+  // process.
+  if (!initialize())
+    return false;
+
----------------
Don't understand the point of this stub function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516