[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao updated this revision to Diff 304182. hliao added a comment. Revise the commit message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https://reviews.llvm.org/D89980 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGenCU

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao updated this revision to Diff 304181. hliao added a comment. Remove aggregate kernel argument coercion only. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https://reviews.llvm.org/D89980 Files: clang/lib/CodeGen/TargetInfo.cpp

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added a comment. In D89980#2372102 , @hliao wrote: > In D89980#2371966 , @arsenm wrote: > >> In D89980#2371952 , @hliao wrote: >> >>> In D89980#2371850

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. In D89980#2371966 , @arsenm wrote: > In D89980#2371952 , @hliao wrote: > >> In D89980#2371850 , @arsenm wrote: >> >>> This should use byref, but I don'

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added a comment. In D89980#2371952 , @hliao wrote: > In D89980#2371850 , @arsenm wrote: > >> This should use byref, but I don't think this should come at the cost of the >> promotion. I would still like to s

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. In D89980#2371850 , @arsenm wrote: > This should use byref, but I don't think this should come at the cost of the > promotion. I would still like to see this promotion occur for the in-memory > byref type Once we use `byref`, that

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added a comment. This should use byref, but I don't think this should come at the cost of the promotion. I would still like to see this promotion occur for the in-memory byref type Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ h

[PATCH] D89980: [hip] Remove kernel argument coercion.

tra added a comment. In D89980#2371580 , @hliao wrote: > is that reported in bugs.llvm.org? It was exposed in our internal code, but the situation is almost identical to your IR example. https://godbolt.org/z/EPPn6h For NVPTX we lower byval arguments as

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. In D89980#2371526 , @tra wrote: > @jlebar -- FYI. This looks pretty similar to the issue you've reported > recently for NVPTX. is that reported in bugs.llvm.org? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D89980: [hip] Remove kernel argument coercion.

tra added a subscriber: jlebar. tra added a comment. @jlebar -- FYI. This looks pretty similar to the issue you've reported recently for NVPTX. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https://reviews.llvm.org/D89980 _

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. The code could be simply converted to a kernel one following the same pattern: struct S { float *p; float a[64]; int n; }; __global__ void kernel(S s) { *s.p = s.a[s.n]; } Here's the LLVM IR after frontend define protec

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added a comment. In D89980#2371270 , @hliao wrote: > In D89980#2368506 , @arsenm wrote: > >> I think this is a dead end approach. I don't see the connection to the >> original problem you are trying to solve

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. In D89980#2368506 , @arsenm wrote: > I think this is a dead end approach. I don't see the connection to the > original problem you are trying to solve. Can you send me an IR testcase that > this is supposed to help? That's probabl

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm requested changes to this revision. arsenm added a comment. This revision now requires changes to proceed. I think this is a dead end approach. I don't see the connection to the original problem you are trying to solve. Can you send me an IR testcase that this is supposed to help? Repos

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. Even GLOBAL may have a better addressing mode, the unpromotable `alloca` resolved in this change has an even significant performance issue. We could favor GLOBAL LOAD/STORE for kernel function as I proposed in other threads but, considering that an aggregate argument may

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added a comment. In D89980#2357208 , @hliao wrote: > Besides the unpromotable `alloca` issue due to indirect accesses, such > coercion to GLOBAL pointer directly is not safe as, in HIP/CUDA, both > CONSTANT and GLOBAL pointers would be passed as t

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. Besides the unpromotable `alloca` issue due to indirect accesses, such coercion to GLOBAL pointer directly is not safe as, in HIP/CUDA, both CONSTANT and GLOBAL pointers would be passed as the kernel arguments. Without introducing a new address space combing GLOBAL/CONSTA

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:19 +// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel1Pi(i32*{{.*}} %x) +// OPT: [[VAL:%.*]] = load i32, i32* %x, align 4 // OPT: [[INC:%.*]] = add nsw i32 [[VAL]], 1 --

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:19 +// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel1Pi(i32*{{.*}} %x) +// OPT: [[VAL:%.*]] = load i32, i32* %x, align 4 // OPT: [[INC:%.*]] = add nsw i32 [[VAL]], 1 -

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:19 +// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel1Pi(i32*{{.*}} %x) +// OPT: [[VAL:%.*]] = load i32, i32* %x, align 4 // OPT: [[INC:%.*]] = add nsw i32 [[VAL]], 1 --

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:19 +// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel1Pi(i32*{{.*}} %x) +// OPT: [[VAL:%.*]] = load i32, i32* %x, align 4 // OPT: [[INC:%.*]] = add nsw i32 [[VAL]], 1 -

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:19 +// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel1Pi(i32*{{.*}} %x) +// OPT: [[VAL:%.*]] = load i32, i32* %x, align 4 // OPT: [[INC:%.*]] = add nsw i32 [[VAL]], 1 --

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:19 +// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel1Pi(i32*{{.*}} %x) +// OPT: [[VAL:%.*]] = load i32, i32* %x, align 4 // OPT: [[INC:%.*]] = add nsw i32 [[VAL]], 1 -

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao updated this revision to Diff 301012. hliao added a comment. Add `amdgpu-kernel-arg-pointer-type.cu` back and revise its checks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https://reviews.llvm.org/D89980 Files: clang/lib/Code

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:30 -} - -// HOST: define void @_Z22__device_stub__kernel2Ri(i32* nonnull align 4 dereferenceable(4) %x) arsenm wrote: > This test should not be deleted. I want to s

[PATCH] D89980: [hip] Remove kernel argument coercion.

arsenm requested changes to this revision. arsenm added inline comments. This revision now requires changes to proceed. Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:30 -} - -// HOST: define void @_Z22__device_stub__kernel2Ri(i32* nonnull align 4 dereferen

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao updated this revision to Diff 300989. hliao added a comment. Revise the comment and point the safety issue by coercing the kernel argument from a generic pointer to a global one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https:

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao added a comment. In D89980#2348339 , @tra wrote: > Are there any tests to illustrate what this change does to IR or generated > code? the existing test `kernel-args.cu` is enhanced by adding a pointer in that aggregate kernel argument. Previously,

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao updated this revision to Diff 300985. hliao added a comment. Test case is enhanced to check that no kernel argument type is coerced. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https://reviews.llvm.org/D89980 Files: clang/lib/

[PATCH] D89980: [hip] Remove kernel argument coercion.

tra added a comment. Are there any tests to illustrate what this change does to IR or generated code? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89980/new/ https://reviews.llvm.org/D89980 ___ cfe-comm

[PATCH] D89980: [hip] Remove kernel argument coercion.

hliao created this revision. hliao added reviewers: arsenm, tra, rjmccall, yaxunl. Herald added subscribers: cfe-commits, kerbowa, nhaehnle, jvesely. Herald added a project: clang. hliao requested review of this revision. Herald added a subscriber: wdng. - If an aggregate argument is indirectly ac