[PATCH] D39137: Add CallSiteSplitting pass

[Jun Bum Lim via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:365
+  if (!splitCallSite(CS, PredBB1, PredBB2, CallInst1, CallInst2)) {
+    if (CallInst1)
+      CallInst1->deleteValue();
----------------
davidxl wrote:
> if splitCallSite returns false, how can CallInst1 be non-null?
CallInst1 or CallInst2 could be created when detecting constrained arguments from an OR condition in createCallSitesOnOrPredicatedArgument(), but SplitBlockPredecessors() not always guarantee splitting predecessors.  In case where we cannot split, we should clean up CallInst1 or CallInst2 if created already. 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:379
+    BasicBlock *BB = &*BI++;
+    for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
+      Instruction *I = &*II++;
----------------
davidxl wrote:
> use range loop.
I don't think we can use range because we modify the IR (remove the call instruction and add blocks) over the iterations.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:382
+      CallSite CS(cast<Value>(I));
+      if (!CS)
+        continue;
----------------
davidxl wrote:
> Skip intrinsicInst. Or if there are folding opportunities, add test cases to cover it.
Added check for IntrinsicInst and InstructionTriviallyDead.  Can you give me little more detail about folding opportunities ? 


https://reviews.llvm.org/D39137