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

[Johannes Doerfert via Phabricator via cfe-commits]

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. 
> > > > 
> > > > How do we know the new store is not inside a loop and might write the value more than once, potentially overwriting a value set later?
> > > > 
> > > > Aside from that (important) question, you need to update the documentation of the function. It doesn't correspond to the new semantics anymore.
> > > > 
> > > I am not convinced with above comments -Because, this code neither changes the address nor the initializer (https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.h#L2548), but only a place where the initialization happens.
> > > 
> > > Further, as the documentation says, the initializer must be a constant or function argument (and surprisingly, I do not see any assertion about it). Hence, even if the initialization happens within the loop, loop invariant code motion pass should detect it.
> > > 
> > > That said, if we think, it is better to keep the initialization within entry block, we can do it at the end of the block.
> > What the initial value is is irrelevant. Arguing LICM should take care of it is also not helpful. Changing the location of the initialization is in itself a problem:
> > 
> > Before:
> > ```
> > a = alloca
> > a = 0;                // init is in the entry block
> > for (...) {
> >   use(a);
> >   a = a + 1;
> > }
> > ```
> > 
> > After, potentially:
> > ```
> > a = alloca
> > for (...) {
> >   a = 0;              // init is now at the builder insertion point
> >   use(a);
> >   a = a + 1;
> > }
> > ```
> Clarification - I am not trying to argue anything here with anybody, I am trying to defend myself, where I feel I am right as per my knowledge. If I realize that I am at false later, then I correct myself, but I am not arguing, and will never ever do.
> 
> Ok, I will try to fix it from the practical point of view.
> 
> But,  I still think as follows - From theoretical/good programming/good front-end code generation perspective, initialization of something will never happen within construct like loop, otherwise initialization has no meaning at all.
> 
> [...] initialization of something will never happen within construct like loop, otherwise initialization has no meaning at all.

Why would we not create a new temporary allocas inside of a loop? There is nothing in any of the APIs or descriptions that would indicate you could not create a temporary alloca and initialize it while you are generating code in a loop. I cannot see why initialization would me meaningless given that it was inserted in the entry block prior to this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110257