[PATCH] D55067: [HIP] Fix offset of kernel argument for AMDGPU target

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D55067#1313290 <https://reviews.llvm.org/D55067#1313290>, @yaxunl wrote:

> In D55067#1313213 <https://reviews.llvm.org/D55067#1313213>, @rjmccall wrote:
>
> > This seems backwards.  Clang knows what the actual ABI alignment of the C type is, and it doesn't have to match the alignment of the IR type that IRGen produces.  It's the actual C ABI alignment that's supposed to affect the calling convention, so there needs to be some way to specify the C ABI alignment on the parameter in IR.  That may mean using `byval`, which can be given an explicit alignment.
>
>
> Also, the alignment of the kernel argument is only used to put the argument in some buffer accessible to the device. It needs not match the C ABI alignment. If the user take address of this argument, they will get address of an alloca, which matches the C ABI alignment.
>
> For example, in code below, %s.coerce is OK to align to 1. If user takes its address, they get %s, which is aligned to 8.
>
>   %struct.S = type <{ i32*, i8, %struct.U, [5 x i8] }>
>   %struct.U = type { i16 }
>  
>   ; Function Attrs: convergent noinline nounwind optnone
>   define amdgpu_kernel void @_Z6kernelc1SPi(i8 %a, %struct.S %s.coerce, i32* %b) #0 {
>   entry:
>     %s = alloca %struct.S, align 8, addrspace(5)
>     %s1 = addrspacecast %struct.S addrspace(5)* %s to %struct.S*
>     %a.addr = alloca i8, align 1, addrspace(5)
>     %0 = addrspacecast i8 addrspace(5)* %a.addr to i8*
>     %b.addr = alloca i32*, align 8, addrspace(5)
>     %1 = addrspacecast i32* addrspace(5)* %b.addr to i32**
>     store %struct.S %s.coerce, %struct.S* %s1, align 8
>     store i8 %a, i8* %0, align 1
>     store i32* %b, i32** %1, align 8
>     ret void
>   }
>  
>


Well the bad thing is that users need to specify different offset for nvptx and amdgpu for the same kernel, if they ever calls {cuda|hip}SetupArgument explicitly.


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

https://reviews.llvm.org/D55067