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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
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
----------------
hsmhsm wrote:
> 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.
Actually we do not just keep removing and recreating llvm.used, it is a converging process. We are only removing variables from there along the route when we replace them. We have to do it before we actually replace a variable because after replaceAllUsesWith() the llvm.used would contain a useless reference to the new structure (and verifier rightfully complaints about that spume).

For the kernel lowering an additional reason to drop it before the replacement is to have isUsedOnlyFromFunction() not to be stopped by a GlobalVariable use. Note that shouldLowerLDSToStruct() has a check that UsedList contains V->stripPointerCasts(), so llvm.used does not prevent the collection. isUsedOnlyFromFunction() does not need this handling because reference is already dropped by the removeFromUsedLists() call here.

For the case where a variable only used by an llvm.used we would not do anything at all since the variable simply will not get into the collection from findVariablesToLower().

We shall also consider the reason why an LDS global may appear in an used list in the first place. The use by a function creates an indirect use by a kernel (at least by an indirectly called function), I do not see any other reason to add the variable to a used list. For a kernel that is simply not needed because a use is direct and immediately visible. In reality this path is very unlikely to be taken in a kernel case, although shall work correctly nonetheless.

Moreover, when we get rid of module LDS, and will have only kernel LDS, plus will manage all the cases all LDS global will be completely replaced and lowered and unconditionally erased from used list. At that point we could just replace this logic with a bulk drop at the top of the runOnModule().

However, while exploring all of that I have realized there is a bug in the implementation, I have to erase a variable from local set UsedList when erasing so we do not have dangling pointers there. Fixed.


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

https://reviews.llvm.org/D102954