[PATCH] D61461: When removing inalloca, convert to static alloca

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

For the multiple call case, we could maybe try to do some sort of control-flow based analysis, instead of trying to analyze uses of the pointer.  Essentially, take advantage of the rule that "the argument allocation must have been the most recent stack allocation that is still live".  But that's probably difficult to implement.



================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2134
+      if (AI->getParent() == &Entry)
+        continue;
+      // Restrict the lifetime of the hoisted alloca to start where the
----------------
rnk wrote:
> inglorion wrote:
> > efriedma wrote:
> > > rnk wrote:
> > > > This will skip the lifetime insertion. Do you think we should do it anyway? I guess it could hypothetically matter if you have a massive single basic block function that does 20 inalloca calls, then stack usage could get out of hand. Maybe just skip the hoisting.
> > > Do we need to check whether the alloca is in the same basic block as the call?  If there's control flow between the alloca and the call, the placement of the lifetime intrinsics might be wrong, or the alloca might still be used by some other inalloca call, or the alloca might not be deallocated along the path where the call doesn't execute.
> > > 
> > > It's easy to construct a C++ testcase where an exception might be thrown between an alloca and the corresponding inalloca call.  Not sure if it's possible for clang or LLVM optimizations to produce an alloca used by multiple different inalloca calls, depending on control flow.
> > This skips the hoisting (that happens in AI->moveBefore(...) later on). This is the case where the alloca is already in the entry block. In that case, I think there is no gain from moving it, and you risk breaking code (by moving it after its use).
> I've been operating under the assumption that stack coloring can tolerate lifetime markers that don't form regions, so this is a best effort to not create excessively large stack frames for functions with long sequences of inalloca calls.
> 
> There are two side exit cases that are interesting:
> - exceptional (common)
> - normal (rare)
> 
> I think normal exits are only possible with gnu statement expressions, which allow you to set up some arguments to a call, and then `return`, `goto`, or `break` out of the call setup. Clang's generated IR will do the stackrestore on a normal side exit, I believe, using the normal cleanup mechanism. We could try to find the associated stacksave / stackrestore calls and use them to insert the lifetime markers, but that requires a lot of analysis. So, I think normal exits are uninteresting.
> 
> I had thought exceptional exits wouldn't be a problem, but now I'm worried about them. When the alloca is dynamic, I believe what happens is that the unwinder resets the stack pointer to what it was at the end of the prologue. This interacts badly with allocas, and MSVC does something to handle this, but I think we never implemented it. However, if we don't place our lifetime end markers along exceptional exits in this transform, I think there's the possibility that we won't do the stack coloring, and we could have runaway stack growth. Hm.
> 
> We could place lifetime end markers along every unwind edge reachable on the path from the alloca to the call... but we'd have to worry about those uncommon normal side exits, then.
The truly awful part about trying to emit lifetime markers for the exception side-exit case is that there isn't any good indication in the IR where, exactly, we would need to place the lifetime.end marker.  The lifetime actually ends at some location in the middle of the exception handler, after the temporary's destructors run.  Maybe clang should emit lifetime markers in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61461