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

[Bob Haarman via Phabricator via llvm-commits]

inglorion 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
----------------
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).


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2138-2141
+          ConstantInt::get(Type::getInt64Ty(AI->getContext()),
+                           Num->getZExtValue() *
+                               F->getParent()->getDataLayout().getTypeAllocSize(
+                                   AI->getAllocatedType()));
----------------
rnk wrote:
> There's a shortcut for this: `IRBuilder<>::getInt64()` can make ConstantInts.
Thanks!


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2147
+          BasicBlock *BB = CS->getSuccessor(i);
+          IRBuilder<>(BB, BB->getFirstInsertionPt())
+              .CreateLifetimeEnd(AI, Size);
----------------
efriedma wrote:
> 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.
I updated the test case so that it hits this case and changed the code to:

 - For non-terminator instructions, insert lifetime.end after.
 - For terminators, look at each of the successors.
   * The base case is to insert before the first non-phi instruction.
   * But if that instruction is a landing pad, insert after it.
   * If there is no instruction after the landing pad, the landing pad is also a terminator,
      and we recurse to its successors.

This should handle the catchswitch case by effectively inserting lifetime.end after the catchpads it dispatches to.


================
Comment at: llvm/test/Transforms/GlobalOpt/inalloca.ll:66
+unwind:
+  landingpad { i8*, i32 } catch i8** null
+; CHECK: %4 = bitcast %struct.a* %a to i8*
----------------
rnk wrote:
> This is a bit artificial, since we don't support generating code for a function that uses landingpad instructions with `__CxxFrameHandler3` personalities. I think it's worth testing catchswitch anyway, but if you want to test landingpad too, add another test that uses `__gxx_personality_v0`.
I cobbled this together, but realized afterwards that it doesn't make much sense. Since this is 32-bit Windows, I rewrote it to use catchswitch instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61461