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

[Mahesha S via Phabricator via cfe-commits]

hsmhsm added a comment.

In D110257#3027633 <https://reviews.llvm.org/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 consequently also reduce their interest in a patch. The recommendation for time without
> review before a "ping" is send is still one week.

Agreed.



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:151
+  // allocas.
+  Builder.CreateStore(Init, Var);
 }
----------------
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.


================
Comment at: clang/test/CodeGenCUDA/builtins-amdgcn.cu:1
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -target-cpu gfx906 -x hip \
----------------
jdoerfert wrote:
> Please prepare a pre-commit that adds auto-generated check lines. Adding them as part of a commit makes it impossible to see the difference.
Will do


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110257