[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/IR/Value.cpp:549
+    if (auto *C = dyn_cast<Constant>(U.getUser())) {
+      if (!isa<GlobalValue>(C)) {
+        C->handleOperandChange(this, New);
----------------
I do not understand, why are we not considering "GlobalVlaue" here. It is upto caller who calls Value::replaceUsesWithIf() to decide on it, and this caller is the one who decide on the logic of the callback of ShouldReplace(). If this caller decide to replace "this" by "New" even in global scope (which is probably an initializer to global), Value::replaceUsesWithIf() should honor it.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:251
     removeFromUsedLists(M, LocalVars);
 
     // Replace uses of ith variable with a constantexpr to the ith field of the
----------------
Further I have my own doubt about the why we actually doing this - removeFromUsedLists().  

I think, it is problamatic even for moduleLDS lower().

When we erase and reconstruct llvm.used/llvm.compiler.used, are we sure that we do not land-up with stale references to (LDS) globals which were added to llvm.used/llvm.compiler.used as used?

As I understand it, the story with "usedList" is as below in this pass:

"When an LDS is used only within llvm.used/llvm.compiler.used, but *nowhere else*, then, we don't want to lower such LDS, that we take care while collecting the globals (for lowering) at the begining. On the other hand, when LDS is used within llvm.used/llvm.compiler.used, but also in other scope (function scope for module lowering), and (kernel scope for kernel lowering), then we want to lower such LDS. That means, we endup replacing LDS within lvm.used/llvm.compiler.used by its struct offset."

If above is what we want to achieve, then why do we need to have call to removeFromUsedLists()? 

As far as I understand, call to removeFromUsedLists() would cause problems for kernel lowering because of repeated erasing and re-constructing of llvm.used/llvm.compiler.used.


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

https://reviews.llvm.org/D102954