[PATCH] D103225: [AMDGPU] Replace non-kernel function uses of LDS globals by pointers.

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:120
+  DenseMap<Function *, SmallPtrSet<GlobalVariable *, 8>> KernelToLDSPointers;
+  DenseMap<Function *, BasicBlock *> KernelToInitBB;
+  DenseMap<Function *, DenseMap<GlobalVariable *, Value *>>
----------------
hsmhsm wrote:
> rampitec wrote:
> > Do you really need this new map? You can get a pointer to block from KernelToLDSPointers by getting a parent of the first instruction.
> I think we need it. Consider below transformed code. Here we need to track the newly introduced block **st** since this is where we need to insert store instructions. And I am not sure, how can I get it from KernelToLDSPointers.
> 
> ```
>     define protected amdgpu_kernel void @k0() {
>     entry:
>       %0 = call i32 @llvm.amdgcn.mbcnt.lo(i32 -1, i32 0)
>       %1 = call i32 @llvm.amdgcn.mbcnt.hi(i32 -1, i32 %0)
>       %2 = icmp eq i32 %1, 0
>       br i1 %2, label %st, label %ft
>     st:
>       store i16 ptrtoint ([4 x i32] addrspace(3)* @lds to i16),  i16 addrspace(3)* @lds.ptr, align 2
>       br label %ft
>     ft:
>       call void @llvm.amdgcn.wave.barrier()
>       call void @f0()
>       ret void
>     }
> ```
Ok.


================
Comment at: llvm/test/CodeGen/AMDGPU/replace-lds-by-ptr-call-diamond-shape.ll:15
+; Pointer should be created.
+; CHECK: @lds_used_within_func.ptr = internal unnamed_addr addrspace(3) global i16 undef, align 2
+
----------------
hsmhsm wrote:
> rampitec wrote:
> > This likely should not be i16. You could do it this way:
> > ```
> > @lds_used_within_func.ptr = internal unnamed_addr addrspace(3) global i8 addrspace(3)* undef, align 2
> > ```
> > Then use bitcast to i8 addrspace(3)* instead of ptrtoint on store.
> > 
> > But I still think that creating module lds right here is better.
> The main goal of using i16 as pointer (with ptrtoint and GEP) is to save memory as much as possible. Using  "i8 addrspace(3)*" means we are going to allocate memory size for pointer (not sure if it is 4 or 8 bytes, but definitely greater than 2 bytes). Hence the use of i16 type here.
> 
> Also, I do not think, it is a good idea to create module lds right here. If that was the case, we could have put this code within module LDS pass itself instead of having separate pass (I did it initially, later we decided that pointer-replacement code should go as separate pass).
> 
> This pass is already too big, let's not change the design again and further complicate the stuff which will further delay code submit, and which leads to other serious problems.
> The main goal of using i16 as pointer (with ptrtoint and GEP) is to save memory as much as possible. Using  "i8 addrspace(3)*" means we are going to allocate memory size for pointer (not sure if it is 4 or 8 bytes, but definitely greater than 2 bytes). Hence the use of i16 type here.

Should not sizeof local pointer be 2? Checked the DL: p3:32:32. Sigh!

> Also, I do not think, it is a good idea to create module lds right here. If that was the case, we could have put this code within module LDS pass itself instead of having separate pass (I did it initially, later we decided that pointer-replacement code should go as separate pass).
> 
> This pass is already too big, let's not change the design again and further complicate the stuff which will further delay code submit, and which leads to other serious problems.

My concern is that you assume these i16 pointers will start a null, but that might not be the case if you rely on the module LDS pass to pack them into the struct along with other LDS and let it sort it.

Can you add a test where you have these pointers and also another module LDS which will have higher alignment and size, so module LDS will pack them together? We could then inspect output of 2 passes and the ISA.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103225