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

Tue Jul 11 04:36:27 PDT 2023

JonChesterfield added a comment.

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?

================
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);
----------------
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?

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:1178
       IRBuilder<> Builder(Ctx);

+      for (Function *Kernel : OrderedKernels) {
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154946