[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 378424. hsmhsm added a comment. Introduce a new instruction pointer which aid all the addressspace casts of static allocas to appear in the source order immediately after all static allocas. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm marked 8 inline comments as done. hsmhsm added a comment. In D110257#3047166 , @rjmccall wrote: > In the other, you're moving the store emitted by `InitTempAlloca` so that it > becomes interleaved with the sequence of `alloca`s, but only when ther

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rjmccall added a comment. This patch seems to be confused. You're making two changes. In one of them, you're trying to prevent `addrspacecast`s from being interleaved with the sequence of `alloca`s in the function prologue. In the other, you're moving the store emitted by `InitTempAlloca` so

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 377742. hsmhsm added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib/CodeGen/CGExpr.cpp clang/test/CodeGenCUDA/builtins-amdgcn.

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 375789. hsmhsm added a comment. Fix review comments by @jdoerfert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib/CodeGen/CGExpr.cpp clang/test/C

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:151 + // allocas. + Builder.CreateStore(Init, Var); } hsmhsm wrote: > jdoerfert wrote: > > hsmhsm wrote: > > > jdoerfert wrote: > > > > I'm not even sure this is necessarily correct. >

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:151 + // allocas. + Builder.CreateStore(Init, Var); } jdoerfert wrote: > hsmhsm wrote: > > jdoerfert wrote: > > > I'm not even sure this is necessarily correct. > > > > > > How do we know

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:151 + // allocas. + Builder.CreateStore(Init, Var); } hsmhsm wrote: > jdoerfert wrote: > > I'm not even sure this is necessarily correct. > > > > How do we know the new store is not in

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added a comment. In D110257#3027633 , @jdoerfert wrote: > While I understand people are eager to receive feedback on their patches, it > is not helpful to ping/remind the reviewers constantly. > This does generate noise for them and can consequent

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

jdoerfert added a comment. While I understand people are eager to receive feedback on their patches, it is not helpful to ping/remind the reviewers constantly. This does generate noise for them and can consequently also reduce their interest in a patch. The recommendation for time without review

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

xgupta added a comment. nice work Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added a comment. @yaxunl / @jdoerfert / @tra Can I expect your further comment/decision on this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 375472. hsmhsm added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib/CodeGen/CGExpr.cpp clang/test/CodeGenCUDA/builtins-amdgcn.

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

xgupta added a comment. Nice work. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:103 +if (!ArraySize) { + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); rnk wrote: > hsmhsm wrote: > > jdoerfert wrote: > > > arsenm wrote:

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:103 +if (!ArraySize) { + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); hsmhsm wrote: > jdoerfert wrote: > > arsenm wrote: > > > Why is there

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 374869. hsmhsm added a comment. Remove "!llvm.access.group" metadata from check lines in test files. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:103 +if (!ArraySize) { + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); jdoerfert wrote: > arsenm wrote: > > Why is there a special AllocaI

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:103 +if (!ArraySize) { + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); arsenm wrote: > Why is there a special AllocaInsertPt iterator i

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

arsenm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:103 +if (!ArraySize) { + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); Why is there a special AllocaInsertPt iterator in the first place?

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rnk added a comment. In D110257#3016918 , @hsmhsm wrote: > That said, it is still good idea (and actually an explicitly not mandated > requirement) to maintain the contiguity of the static allocas at the top of > the basic block as one cluster, and it s

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added inline comments. Comment at: clang/test/OpenMP/distribute_parallel_for_simd_if_codegen.cpp:264 // CHECK1: omp.inner.for.cond: -// CHECK1-NEXT:[[TMP7:%.*]] = load i32, i32* [[DOTOMP_IV]], align 4 -// CHECK1-NEXT:[[TMP8:%.*]] = load i32, i32* [[DOTOMP_UB

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added inline comments. Comment at: clang/test/OpenMP/distribute_parallel_for_simd_if_codegen.cpp:264 // CHECK1: omp.inner.for.cond: -// CHECK1-NEXT:[[TMP7:%.*]] = load i32, i32* [[DOTOMP_IV]], align 4 -// CHECK1-NEXT:[[TMP8:%.*]] = load i32, i32* [[DOTOMP_UB

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

yaxunl added a comment. Pls make sure the patch passes internal CI. Thanks. Comment at: clang/test/OpenMP/distribute_parallel_for_simd_if_codegen.cpp:264 // CHECK1: omp.inner.for.cond: -// CHECK1-NEXT:[[TMP7:%.*]] = load i32, i32* [[DOTOMP_IV]], align 4 -// CHECK1-NE

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 374486. hsmhsm added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib/CodeGen/CGExpr.cpp clang/test/CodeGenCUDA/builtins-amdgcn.

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:102-106 + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); + if (Iter != EBB->end()) +++Iter; + Builder.SetInsertPoint(EBB, Iter); -

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 374419. hsmhsm added a comment. Update source comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib/CodeGen/CGExpr.cpp clang/test/CodeGenCUDA/b

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added a comment. In D110257#3016113 , @rnk wrote: > In D110257#3015641 , @hsmhsm wrote: > >> The logic at >> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L209

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rnk added a comment. In D110257#3015641 , @hsmhsm wrote: > The logic at > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2093 > assumes that static allocas (within callee) are contiguous. No, it doesn't mak

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

jdoerfert added a subscriber: rnk. jdoerfert added a comment. In D110257#3015712 , @jdoerfert wrote: > In D110257#3015641 , @hsmhsm wrote: > >> In D110257#3015553 , @arsen

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

arsenm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:102-106 + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); + if (Iter != EBB->end()) +++Iter; + Builder.SetInsertPoint(EBB, Iter); -

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

jdoerfert added a comment. In D110257#3015641 , @hsmhsm wrote: > In D110257#3015553 , @arsenm wrote: > >> I do think it's cleaner/more canonical IR to cluster these at the top of the >> block, but I don't underst

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm added a comment. In D110257#3015553 , @arsenm wrote: > I do think it's cleaner/more canonical IR to cluster these at the top of the > block, but I don't understand this comment: > >> otherwise, inliner's attempt to move static allocas from callee

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

arsenm added a comment. I do think it's cleaner/more canonical IR to cluster these at the top of the block, but I don't understand this comment: > otherwise, inliner's attempt to move static allocas from callee to caller > will fail, The inliner successfully moves allocas to the caller's entry

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

hsmhsm updated this revision to Diff 374252. hsmhsm added a comment. Update source comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: clang/lib/CodeGen/CGExpr.cpp clang/test/CodeGenCUDA/b