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

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 6 22:26:11 PDT 2021


rjmccall added a comment.

This patch seems to be confused.  You're making two changes.  In one of them, you're trying to prevent `addrspacecast`s from being interleaved with the sequence of `alloca`s in the function prologue.  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`.

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.


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