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

Mahesha S via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 6 23:08:37 PDT 2021


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



More information about the cfe-commits mailing list