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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
