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

[Mahesha S via Phabricator via cfe-commits]

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 AllocaInsertPt iterator in the first place? Can you avoid any iteration logic by just always inserting at the block start?
> Right. The alloca insertion point is sometimes changed to a non-entry block, and we should keep that ability.
> From all the use cases I know it would suffice to insert at the beginning of the alloca insertion point block though.
I really do not understand this comment fully.

This block of code here inserts an "addressspace cast" of recently inserted alloca, not the alloca itself. Alloca is already inserted.  Please look at the start of this function. 

The old logic (in the left) inserts addressspace cast of recently inserted alloca immediately after recent alloca using AllocaInsertPt. As a side effect, it also makes AllocaInsertPt now point to this newly inserted addressspace cast. Hence, next alloca is inserted after this new addressspace cast, because now AllocaInsertPt is made to point this newly inserted addressspace cast.  

The new logic (here) fixes that by moving insertion point just beyond current AllocaInsertPt without updating AllocaInsertPt.

How can I insert "addressspace cast" of an alloca, at the beginning of the block even before alloca?

As I understand it, AllocaInsertPt is maintained to insert static allocas at the start of entry block, otherwise there is no any special reason to maintain such a special insertion point. Please look at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.h#L378.

That said, I am not really sure, if I have completely misunderstood the comment above. If that is the case, then, I need better clarification here about what really is expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110257