hsmhsm marked 8 inline comments as done. hsmhsm added a comment. In D110257#3047166 <https://reviews.llvm.org/D110257#3047166>, @rjmccall wrote:
> In the other, you're moving the store emitted by `InitTempAlloca` so that it > becomes interleaved with the sequence of `alloca`s, but only when there's an > `addrspacecast`. Not really. In the absence of this patch, `addrspacecast` are interleaved with the sequence of `alloca`s, and `AllocaInsertPt` always point to the end of this *interleaved sequence*. Within InitTempAlloca(), any init to alloca (or to its `addrspacecast` in case of an addrspacecast) happens just after `AllocaInsertPt` which is fine. Now, in the presence of this patch, `AllocaInsertPt` points to the end of contiguous `alloca`s but *BEFORE* any addrspacecast. This forces the changes to InitTempAlloca(). Otherwise, init of `addrspacecast` happens before `addrspacecast` itself. > Now, I should say that `InitTempAlloca` is generally a problematic function. > Its purpose is to put an initialization in the prologue of the function so > that it always happens prior to some other code executing. This is rarely > useful, though, because the memory is usually tied to some specific feature > in the code, and as Johannes says, that place in the code may be reachable > multiple times, and the memory typically needs to be freshly initialized each > time. Using `InitTempAlloca` is therefore frequently wrong, and I'm honestly > not sure there's any good reason to use it. Looking at the calls, some of > them know that they're in the prologue, and so it should be fine to simply > emit a store normally. Other calls seem quite suspect, like the one in > CGObjCGNU.cpp. And even if it's semantically okay, it's potentially doing > unnecessary work up-front when it only really needs to happen if that path is > taken. > > So I don't really care about aesthetic differences in the code created by > `InitTempAlloca`, because we should just remove it completely. > > If we really care about not interleaving things with the `alloca` sequence — > and right now I am not convinced that we do, because contra the comments and > description of this patch, this is not an LLVM requirement of any sort — I > think we should lazily create a second InsertPt instruction after the > `AllocaInsertPt` and insert all the secondary instruction prior to that so > that they appear in source order. Agree. I will give it a try to make changes as you suggest, though it may take some time since it requires a bit of cleanup and handling of the changes to (possibly many) lit tests as a side effect. 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
