[PATCH] D103225: [AMDGPU] Replace non-kernel function uses of LDS globals by pointers.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:29
+// type which would cause allocating huge memory for struct instance within
+// every kernel. Hence, before runnning this pass, it is advisable to run the
+// pass "amdgpu-replace-lds-use-with-pointer" which will replace LDS uses within
----------------
Typo runnning


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:147-150
+    // LDS whose size is very small and doesn`t exceed pointer size is not worth
+    // replacing.
+    if (DL.getTypeAllocSize(GV->getValueType()) <= 2)
+      return true;
----------------
Why not check this first?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:200
+    IRBuilder<> Builder(EI);
+    Builder.CreateStore(Builder.CreatePtrToInt(GV, Type::getInt16Ty(Ctx)),
+                        LDSPointer);
----------------
The compiler should never introduce ptrtoint. This should offset from a base address with getelementptr


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:243
+
+void convertConstExprsToInstructions(
+    Instruction *I, SmallPtrSetImpl<Value *> &Operands,
----------------
This should really be a generic utility function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103225