[PATCH] D43729: [CallSiteSplitting] properly split musttail calls

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for updating! I left a few more comments, but it looks good overall. I just left a few thoughts about structuring the code to deal with musttail in splitCallSite. Sorry if I was not too clear earlier. Anyways, those are mostly my personal preference. Also, I think doing the deletion after the loop is totally fine and avoids problems.



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:215
+  Instruction *Copy = I->clone();
+  Copy->setName(I->getName());
+  Copy->insertBefore(Before);
----------------
I don't think we need to do that.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:278
+  if (IsMustTailCall) {
+    auto II = std::next(Instr->getIterator());
+
----------------
Personally I would just move the code in this block and the code for the musttail handling in the main loop to a single function, named something like addMusttailReturn. It means we might end up doing a bit of additional work, but it keeps splitCallSite simpler.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:283
+      ++II;
+      assert(!IsVoid && "bitcast after void musttail call");
+    }
----------------
Not sure if we need those assertions here. If anything is wrong here, then it should have been invalid earlier too.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:289
+        "`musttail` call must be followed by `ret` instruction");
+    assert(std::next(II) == TailBB->end() && "`ret` doesn't end the block");
+  } else if (Instr->getNumUses())
----------------
Not sure if we need those assertions here. If anything is wrong here, then it should have been invalid earlier too.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:327
+    if (IsMustTailCall) {
+      TerminatorInst *TI = SplitBlock->getTerminator();
+      Value *V = NewCI;
----------------
I think it would be slightly better to have that code a function addMusttailReturn or something.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:333
+
+      // FIXME: remove TI here, once https://reviews.llvm.org/D43822 lands
+    }
----------------
I don't think D43822 fixes the problem. For now I think it's OK to just remove the terminators  after the main loop (and drop the fixme).


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:350
+  // Erase the tail block once done with musttail patching
+  if (IsMustTailCall) {
+    TailBB->eraseFromParent();
----------------
Please merge that with the block that deletes the terminators. I would drop the fixme and add a comment with a note that we delete it after we split all predecessors to avoid iterator invalidation.


https://reviews.llvm.org/D43729