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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

You probably need to wrap all prologue LDS stores into a block to execute it only from lane 0 and add a barrier after. @t-tye correct me if I am wrong.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:310
+                      false, false)
+INITIALIZE_PASS_DEPENDENCY(AMDGPUReplaceLDSUseWithPointer)
+INITIALIZE_PASS_END(AMDGPULowerModuleLDS, DEBUG_TYPE,
----------------
It is not strictly required. I would just add it from TM.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:122
+  std::vector<GlobalVariable *> collectLDSRequiringPointerReplace() {
+    SmallPtrSet<GlobalValue *, 32> UsedList = AMDGPU::getUsedList(M);
+
----------------
auto UsedList. Less updates if return type will change.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:135
+
+    UsedList.clear();
+
----------------
It is local variable, no need to clean. It will be destroyed.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:140
+
+  // Returns true if an LDS global does not require its non-kernel function uses
+  // to be replaced by pointer.
----------------
Require is too strong word here. Not beneficial.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:151
+    LDSToNonKernels[GV] = AMDGPU::collectNonKernelAccessorsOfLDS(GV);
+    if (LDSToNonKernels[GV].empty())
+      return true;
----------------
return !LDSToNonKernels[GV].empty();


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:168
+    //
+    // Each CU owns 64K of LDS memory, so address ranges from 0 to 2^16 - 1.
+    // Hence 16 bit pointer is enough to hold the LDS address.
----------------
Technically it can be less. Some ASICs have 32K.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:262
+    if (!Entry.second) {
+      auto Entry2 = Entry.first->second.insert(std::make_pair(GV, nullptr));
+      if (!Entry2.second)
----------------
Can you name it somehow more readable? Entry, Entry2... Not speaking names.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:309
+  // Holds all those non-kernel functions within which LDS is being accessed.
+  SmallPtrSet<Function *, 8> LDSAccessors = LDSToNonKernels[GV];
+
----------------
That's a copy of set, you could use reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103225