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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
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:
> 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.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2147
+          BasicBlock *BB = CS->getSuccessor(i);
+          IRBuilder<>(BB, BB->getFirstInsertionPt())
+              .CreateLifetimeEnd(AI, Size);
----------------
rnk wrote:
> It's unfortunately very likely that `getFirstInsertionPt` may return an end iterator if BB is a `catchswitch` BB. I would just continue the loop in these cases. A missing lifetime end should pessimize the code, not lead to miscompiles. You should be able to construct a test case for this by putting an inalloca call inside a try / catch.
There might not be an insertion point in a block with a catchswitch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61461