[PATCH] D154946: [AMDGPULowerModuleLDSPass] Kernels do not always have entries in KernelToReplacement map

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

jmmartinez added a comment.

In D154946#4488779 <https://reviews.llvm.org/D154946#4488779>, @JonChesterfield wrote:

> Thanks for the patch! On reading through I think there's something more fundamentally broken here. Please could you hold fire on this for a moment while I dig through it?
> There's some refactoring in line which seems worth having asap - the extra const, the spurious copy deleted - would you prefer I post a diff with those parts pulled out or you post one for me to accept?

No problem! I will move it into a separate patch.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:486
     for (size_t i = 0; i < Variables.size(); i++) {
       GlobalVariable *GV = Variables[i];
+      auto ConstantGepIt = LDSVarsToConstantGEP.find(GV);
----------------
JonChesterfield wrote:
> I don't follow this chunk. Previously count != 0 followed by lookup. New find != end followed by using the previous value. Is there a functional change I'm missing here?
I'll move it out in another patch. It is a consequence of adding the const.

With const, we cannot use operator[] since it inserts a default initialized element if none exist (the count was guarding against it).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:1178
       IRBuilder<> Builder(Ctx);
 
+      for (Function *Kernel : OrderedKernels) {
----------------
JonChesterfield wrote:
> Nervous about this. There's a bunch of equality relationships between structures in this pass and the IR generated, probably not adequately documented.
> 
> Some number of kernels get identified as needing LDS related stuff and those get put in the OrderedKernels vector. There's an intrinsic which amounts to an index into that vector which persists beyond this pass. If KernelToReplacement is missing an entry for one of those kernels, I've botched the control flow elsewhere in this function.
If I'm not wrong, this happens since `lowerKernelScopeStructVariables` doesn't add replacements for Dynamic LDS variables.

Kernels accesing only Dynamic LDS still need an id for the lookup table., even if they don't have any static LDS kernel variable.

(I hope I'm not mixing concepts)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154946