[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:
> > 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;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110257