[PATCH] D103431: [AMDGPU] Fix missing lowering of LDS used in global scope.

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:99
 
-    if (auto *G = dyn_cast<GlobalValue>(V->stripPointerCasts())) {
-      if (UsedList.contains(G)) {
-        continue;
+    if (auto *G = dyn_cast<GlobalVariable>(V)) {
+      StringRef GName = G->getName();
----------------
hsmhsm wrote:
> rampitec wrote:
> > stripPointerCasts() still makes sense.
> No, as I understand it, we should be very carefull when using stripPointerCasts(), otherwise, it will lead to unexpected and surprising behavior. 
> 
> For exmaple, in this case, let's try below simple example. Here, only user of "@lds.1" is bitcast inst within kernel. Now, when we are analysing this user, we apply stripPointerCasts(), which will return back "@lds.1" itself, and since it is a global variable, the IF condition satisfies, and we incorrectly land within IF block, where we did not want, which leads to incorrect transormation.
> 
> ```
> @lds.1 = internal unnamed_addr addrspace(3) global [1 x i8] undef, align 1
> 
> define amdgpu_kernel void @k0() {
>   %bc = bitcast [1 x i8] addrspace(3)* @lds.1 to i8 addrspace(3)*
>   store i8 1, i8 addrspace(3)* %bc, align 1
>   ret void
> }
> ```
Ack.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:135
+    for (auto *G : GlobalUsers)
+      Ret |= hasUserInstruction(G);
   }
----------------
hsmhsm wrote:
> rampitec wrote:
> > How can you get here? You should have inspected all the users by this time and met Instruction uses if any.
> Please note that the global(s) that we are trying to inspect here are not lds (GV) itself, but, they are the ones which use lds (GV) as an initializer in the global scope or they are special llvm.used. We collect such globals which use lds (GV), and decide whether to lower or not based on the use of those globals.
> Please note that the global(s) that we are trying to inspect here are not lds (GV) itself, but, they are the ones which use lds (GV) as an initializer in the global scope or they are special llvm.used. We collect such globals which use lds (GV), and decide whether to lower or not based on the use of those globals.

Ack. Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103431