[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rnk added a comment. In D110257#3134001 , @JonChesterfield wrote: > So you won't articulate or document the new invariant and you think there's a > llvm-dev discussion that says we can't verify the invariant which you won't > reference, but means you w

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. If the amdgpu backend doesn't require this then it doesn't matter much if other passes undo it. If it's not an invariant, we don't need the verifier to alert people to passes that break it. Git blame on the new code in clang will lead people here where they may

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

arsenm added a comment. In D110257#3134001 , @JonChesterfield wrote: > So you won't articulate or document the new invariant and you think there's a > llvm-dev discussion that says we can't verify the invariant which you won't > reference, but means yo

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

arsenm added a comment. In D110257#3133866 , @hsmhsm wrote: > This is not something specific to AMDGPU backend, but AMDGPU backend at > present requires this canonical form. I must emphasize this is not a hard requirement, just a nice to have Reposit

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. So you won't articulate or document the new invariant and you think there's a llvm-dev discussion that says we can't verify the invariant which you won't reference, but means you won't add this to the verifier. Request changes doesn't really work after you've ap

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm added a comment. In D110257#3133934 , @JonChesterfield wrote: > In D110257#3133895 , @hsmhsm wrote: > >> In D110257#3133879 , >> @JonChesterfield wrote: >> >>> In

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. In D110257#3133895 , @hsmhsm wrote: > In D110257#3133879 , > @JonChesterfield wrote: > >> In D110257#3133866 , @hsmhsm wrote: >> >>> This

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm added a comment. In D110257#3133879 , @JonChesterfield wrote: > In D110257#3133866 , @hsmhsm wrote: > >> This is not something specific to AMDGPU backend, but AMDGPU backend at >> present requires this c

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. In D110257#3133866 , @hsmhsm wrote: > This is not something specific to AMDGPU backend, but AMDGPU backend at > present requires this canonical form. Undocumented and not checked by the IR verifier. Canonical form seems

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm added a comment. In D110257#3133838 , @JonChesterfield wrote: > Please change the commit message to say why this change is necessary / an > improvement on what we have now. > > My recollection is that the amdgpu backend crashes on some IR and thi

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. Please update the documentation with this new constraint. It would be helpful to know exactly when we now require alloca instructions to be adjacent to one another. If you wish to avoid other passes breaking the invariant in future, I think it needs to be added

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. Please change the commit message to say why this change is necessary / an improvement on what we have now. My recollection is that the amdgpu backend crashes on some IR and this decreases the probability of that IR pattern occuring, which still sounds like fixi

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

This revision was automatically updated to reflect the committed changes. Closed by commit rG3b9a85d10ac7: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas (authored by hsmhsm). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm added a comment. Thanks @rnk and @yaxunl . 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:

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I believe you have addressed John's comments, and I think the IR changes mainly affect AMDGPU users, so I don't think this will be too disruptive. Sorry about the delay, there's a bit of a bys

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

yaxunl added a comment. LGTM. It seems all concerns have been addressed. Shall we move ahead and land this patch? Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 _

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm added a comment. This patch is waiting for an action for a long time. I am expecting at-least anyone of the reviewers to either say "yes" or "no" or "further required improvements" on this patch so that I can take further action on this patch. If you say "no" to this patch with a strong

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm updated this revision to Diff 384661. 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/lib/CodeGen/CodeGenFunction.cpp

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm added a comment. @rjmccall I assume, I have fixed all your review comments. In case, if I have missed something OR if you think, few more changes are required for the patch, please do let me know so that I will proceed as per the comments/suggestions. I would like to bring this patch to

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the 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] Make sure to maintain the contiguity of all the static allocas

hsmhsm updated this revision to Diff 380871. 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/lib/CodeGen/CodeGenFunction.cpp

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm updated this revision to Diff 380320. 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/lib/CodeGen/CodeGenFunction.cpp

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm updated this revision to Diff 379918. hsmhsm marked 2 inline comments as done. hsmhsm added a comment. Rebase + minor clean-up to patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 Files: cla

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm marked 2 inline comments as done. hsmhsm added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:115 +if (AllocaInsertedAtAllocaInsertPt) + AllocaAddrSpaceInsertPt = dyn_cast(V)->getIterator(); } arichardson wrote: > Shouldn't this use

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm updated this revision to Diff 379682. hsmhsm added a comment. Fix review comments by @rjmccall. 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/lib/Cod

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

arichardson added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:115 +if (AllocaInsertedAtAllocaInsertPt) + AllocaAddrSpaceInsertPt = dyn_cast(V)->getIterator(); } Shouldn't this use `cast` instead? Repository: rG LLVM Github Monorepo

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:392 + /// order immediately after all static allocas. + llvm::BasicBlock::iterator AllocaAddrSpaceInsertPt; + Please call this something like `PostAllocaInsertPt`. Instead of e

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

hsmhsm updated this revision to Diff 378875. 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/lib/CodeGen/CodeGenFunction.cpp