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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Nice!



================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2126
+        StripAttr(F->getContext(), CS.getAttributes(), Attribute::InAlloca));
+    if (AllocaInst *AI = dyn_cast<AllocaInst>(CS.getArgument(ArgNo))) {
+      AI->setUsedWithInAlloca(false);
----------------
I would suggest adding `->stripPointerCasts()` to the argument here. In LTO scenarios with C++ templates, it's highly likely that two different TUs will end up computing different struct types that are structurally equivalent. When they are linked together, pointer bitcasts may be added to make the types work out.


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


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


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2147
+          BasicBlock *BB = CS->getSuccessor(i);
+          IRBuilder<>(BB, BB->getFirstInsertionPt())
+              .CreateLifetimeEnd(AI, Size);
----------------
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.


================
Comment at: llvm/test/Transforms/GlobalOpt/inalloca.ll:66
+unwind:
+  landingpad { i8*, i32 } catch i8** null
+; CHECK: %4 = bitcast %struct.a* %a to i8*
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61461