[PATCH] D102954: [AMDGPU] Lower kernel LDS into a sorted structure

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:39
+    return all_of(U->users(), [F](const User *U) {
+      return isUsedOnlyFromFunction(U, F);
+    });
----------------
For me, this logic looks incomplete in general sense. Constants are constants throughout compilation phase, meaning, once a constant is created, it is reused in multiple contexts when required. So this is possible case.

void kern_0() {
  Inst_1(Const_1(Const_3(LDS)));
}

void kern_1() {
  Inst_1(Const_2(Const_3(LDS)));
}

Here Inst_1 and Inst_2 are two different instances, even if they are exactly same, but Const_3 is one-and-the same for both kern_0() and kern_1().  Will this case works here?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:47
+bool shouldLowerLDSToStruct(const SmallPtrSetImpl<GlobalValue *> &UsedList,
+                            const GlobalVariable &GV, const Function *F) {
   // Any LDS variable can be lowered by moving into the created struct
----------------
This is fine, but combining the testing of two different requirements for both module lowering and kernel lowering within same function looks maintenance headache to me. May be it is useful to separate them into two different functions, and findVariablesToLower() call one of them, based on F. But it is upto you. I am fine either way.


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

https://reviews.llvm.org/D102954