[PATCH] D138141: [amdgpu] Reimplement LDS lowering

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:477
+
+    return {direct_map_kernel, indirect_map_kernel};
+  }
----------------
I might be wrong, but I think the copy-elision is done for the newly created pair, but not for the arguments used for create it.

I'd rather do:
```
return {std::move(direct_map_kernel), std::move(indirect_map_kernel)};
```

Without move: https://godbolt.org/z/f5Ghr8M1o
With move: https://godbolt.org/z/WeG7Y8nY8


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:491
+  static Constant *getAddressesOfVariablesInKernel(
+      LLVMContext &Ctx, std::vector<GlobalVariable *> Variables,
+      DenseMap<GlobalVariable *, Constant *> &LDSVarsToConstantGEP) {
----------------
Is there a missing const& for the Variables argument ?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:517
+      Module &M, std::vector<GlobalVariable *> const &Variables,
+      std::vector<Function *> kernels,
+      DenseMap<Function *, LDSVariableReplacement> &KernelToReplacement) {
----------------
Could you use a const& for kernels ?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:735-738
+      DenseSet<Function *> const &HybridModuleRootKernels =
+          HybridModuleRoot
+              ? LDSToKernelsThatNeedToAccessItIndirectly[HybridModuleRoot]
+              : DenseSet<Function *>{};
----------------
In the case HybridModuleRoot is null, the reference HybridModuleRootKernels will point to a temporary that has already been destroyed.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:1154
+  void replaceLDSVariablesWithStructVec(
+      Module &M, std::vector<GlobalVariable *> const &LDSVarsToTransformArg,
+      LDSVariableReplacement Replacement, PredicateTy Predicate) {
----------------
What do you think about taking an r-value reference for LDSVarsToTransformArg. That would avoid the copy into the local variable LDSVarsToTransform.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:176-177
+
+      Constant *C = Constant::getIntegerValue(
+          IntegerType::get(CV->getContext(), 32), APInt(32, offset));
+      return AsmPrinter::lowerConstant(C);
----------------
Cosmetic: There is a shorter [[ https://llvm.org/doxygen/classllvm_1_1ConstantInt.html#a4757205dd7df6b811f16d2bec12c46d8 | ConstantInt::get ]] that takes directly an APInt and an LLVMContext.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138141