[PATCH] D138141: [amdgpu] Reimplement LDS lowering

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I think we need to get some documentation explaining the LDS lowering flow and the options. Not sure where the best place for it; I guess AMDGPUUsage?



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:87
 
+static const char ModuleLDSName[] = "llvm.amdgcn.module.lds";
+
----------------
I think you're supposed to use StringLiteral these days for some reason 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:98
+  const Module &M = *GV.getParent();
+  std::string NS{GV.getName()};
+  StringRef N(NS);
----------------
Don't see the point of this string copy, GV.getName() gives you a StringRef to begin with 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:126
+  assert(isKnownAddressLDSGlobal(GV));
+  unsigned offset = 0;
+
----------------
Capitalize 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:128
+
+  if (GV.getName().equals(ModuleLDSName)) {
+    return 0;
----------------
Why not just ==?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:111-112
+  // LDS allocation should have an associated kernel function
+  static const Function *
+  getKernelLDSFunctionFromGlobal(const GlobalVariable &GV);
   static const GlobalVariable *
----------------
These aren't really gaining anything from being part of AMDGPUMachineFunction, but I guess it's where the other stuff already lives


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6015
+        dyn_cast<const GlobalVariable>(GA->getGlobal())) {
+      CanFold |= AMDGPUMachineFunction::isKnownAddressLDSGlobal(*GV);
+    }
----------------
Could just early return on each of these instead of or'ing 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138141