[PATCH] D152431: [Inliner] Handle convergence control when inlining a call

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Thanks, I like this much more :)



================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:2303-2312
+  if (ConvergenceControlToken) {
+    auto *I = FirstNewBlock->getFirstNonPHI();
+    if (auto *IntrinsicCall = dyn_cast<IntrinsicInst>(I)) {
+      if (IntrinsicCall->getIntrinsicID() ==
+          Intrinsic::experimental_convergence_entry) {
+        IntrinsicCall->replaceAllUsesWith(ConvergenceControlToken);
+        IntrinsicCall->eraseFromParent();
----------------
sameerds wrote:
> nhaehnle wrote:
> > This looks like it's making an assumption that the `convergence.entry` is the very first instruction. Do we really want to enforce that? Definitely needs a verifier check in that case...
> Yes. We enforce it in D147116.
Okay thanks, I missed that the first time around.

This needs a little more thought on the various `getFirstWhatever` methods in `BasicBlock` though. My main immediate concern is with `IRBuilder::setInsertPointPastAllocas`, which uses `BasicBlock::getFirstNonPHIOrDbgOrAlloca`. We'd want this to set the insert point past the entry/loop intrinsic and any allocas.

I wouldn't be surprised if some of the other helpers there should also be adjusted -- for example, `getFirstInsertionPt` -- but that is less clear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152431