[PATCH] D94648: [amdgpu] Implement lower function LDS pass

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added a comment.

In D94648#2770088 <https://reviews.llvm.org/D94648#2770088>, @hsmhsm wrote:

> Hi Jon,
>
> For the 0 argument kernel, did you put debug statement within SITargetLowering::LowerFormalArguments() and test whether it will hit or not?  My experimentation shows that it is indeed hit for 0 arg kernels too. So it is not problem with 0 arg kernel.

Could be. I'm very limited in the time I can spend on this, debugging was a few spare minutes.

> Problem is within the function - AMDGPUMachineFunction::allocateModuleLDSGlobal() that you wrote.
>
> The statement
>
>   GlobalVariable *GV = M->getGlobalVariable("llvm.amdgcn.module.lds");
>
> is suppose to be replaced by
>
>   GlobalVariable *GV = M->getGlobalVariable("llvm.amdgcn.module.lds", true);

API docs agree. This will be a problem in AMDGPULowerModuleLDS::removeFromUsedList as well, the getGlobalVariable at the entry to the function will be ignoring internal variables, so also needs the `,true`.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:179
+    for (size_t I = 0; I < LocalVars.size(); I++) {
+      if (Constant *C = dyn_cast<Constant>(LocalVars[I]->stripPointerCasts())) {
+        LocalVarsSet.insert(C);
----------------
hsmhsm wrote:
> Every GlobalVariable should be Constant. ref - https://llvm.org/doxygen/classllvm_1_1Constant.html. Then, why do we need dyn_cast<>, and an if conditional check here? Cannot we direct cast<> to Constant?
We don't need dyn_cast here, cast is fine


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:303
+        CurrentOffset += DL.getTypeAllocSize(FGV->getValueType());
+      }
+    }
----------------
hsmhsm wrote:
> Will the logic within above *for* loop ensures that first *non-padding* (real) member of the struct will be accessed at the same address as that of struct instance?
it'll be at zero, so, yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94648