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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:224
+  PHINode *PN = dyn_cast<PHINode>(&*BI);
+  if (PN != nullptr)
+    ++BI;
----------------
You can change "if (PN != nulltr)" into "if (PN)".

Isn't it possible to set isVoid here? 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:236
+  ReturnInst *RI = dyn_cast<ReturnInst>(&*BI);
+  assert(RI != nullptr &&
+      "`musttail` call must be followed by `ret` instruction");
----------------
assert(RI != nullptr &&   -> assert(RI && 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:240
+
+  assert(++BI == TailBB->end() && "`ret` doesn't end the block");
+
----------------
Although BI is no longer used, ++BI in assert doesn't seem to be a good idea.  std::next(BI) should be better. 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:250
+
+    CallInst *CI = dyn_cast<CallInst>(&*Split->begin());
+    assert(CI != nullptr && "pred blocks must have the call");
----------------
It's not always true because we can have some instructions before the call-site split (e.g., callsite-instructions-before-call.ll).  



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:254
+
+    bool IsVoid = CI->getFunctionType()->getReturnType()->isVoidTy();
+    Value *V = CI;
----------------
We don't need to do this in the loop.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:258-267
+    if (BCI != nullptr) {
+      assert(!IsVoid && "Can\'t bitcast void");
+      Instruction *NewBCI = BCI->clone();
+      NewBCI->setName(BCI->getName());
+      NewBCI->insertBefore(TI);
+      NewBCI->setOperand(0, V);
+      DEBUG(dbgs() << "        " << *NewBCI << " into\n");
----------------
Look like we can make a separate function for cloning. 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:286
+  // Remove unused instructions
+  if (RI != nullptr)
+    RI->eraseFromParent();
----------------
if (RI) should be better.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:414
 
+  if (CS.isMustTailCall())
+    fixMustTailCallSite(TailBB);
----------------
The original CallInst must be removed in above while loop (line 394). 


https://reviews.llvm.org/D43729