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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked an inline comment as done.
sameerds added inline comments.


================
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();
----------------
nhaehnle wrote:
> 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.
Sure. We want to incorporate the rules for these intrinsics in all the convenient places, we can do that as a follow-up, right? This change solely focuses on inlining of function calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152431