[PATCH] D94648: [amdgpu] Implement lower function LDS pass

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLDSPass.cpp:69
+                        const Use &U, uint64_t Depth = 0) {
+    // Variables used by kernels/.used are not moved into the new struct
+    User *V = U.getUser();
----------------
Erroneous characters in the comment.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLDSPass.cpp:201
+
+  bool runOnModule(Module &M) override {
+    if (DisableLowerFunctionLDS) {
----------------
Can we please split this function into logical blocks and wrap them in private functions? That would make the code more readable.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerFunctionLDSPass.cpp:218
+    if (FoundLocalVars.empty()) {
+      // No variables to rewrite, no changes made.
+      return false;
----------------
Such information is useful to know, so please use debugging messages with DEBUG_TYPE. Debugging messages would help us to spot issues.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:43
+  const Module *M = F.getParent();
+  GlobalVariable *GV = M->getGlobalVariable("__function_lds");
+  if (GV) {
----------------
I think this name can be a bit more mangled. It is easy to have a lier in the file. Probably use mechanism to randomly generate a string and use that to name and use the same random algorithm while de-referencing. This is too fancy but a bit more mangled name should be used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94648