[PATCH] D39137: Add CallSiteSplitting pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:546
+
+  if (Level == O3)
+    EarlyFPM.addPass(CallSiteSplittingPass());
----------------
Is there any reason you use Level == 3 here and Level > 2 in lib/Transforms/IPO/PassManagerBuilder.cpp?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:77
+static void addNonNullAttribute(Instruction *CallI, Instruction *&NewCallI,
+                                Value *Op, Constant *ConstValue) {
+  if (!NewCallI)
----------------
ConstValue seems unused?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:82
+  unsigned ArgNo = 0;
+  for (CallSite::arg_iterator I = CS.arg_begin(), E = CS.arg_end(); I != E;
+       ++I, ++ArgNo)
----------------
A range-based-for loop might make things easier here.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:115
+           "Expected predecessor block to end in a conditional branch.");
+    ICmpInst *Cmp = cast<ICmpInst>(PBI->getCondition());
+    Value *Op0 = Cmp->getOperand(0);
----------------
Would it make sense to add an assertion making sure `PBI->getCondition()` is an ICmpInst? I assume it could be a FCmpInst as well, if the caller passes a wrong BranchInsts. Same forthe cast to `Constant` later on.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:116
+    ICmpInst *Cmp = cast<ICmpInst>(PBI->getCondition());
+    Value *Op0 = Cmp->getOperand(0);
+    Constant *Op1 = cast<Constant>(Cmp->getOperand(1));
----------------
I think Op0 and Op1 could do with more expressive names. Op0 is the argument that gets replaced by the constant Op1, right?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:144
+      }
+
+    } else {
----------------
You could use continue here to get rid of the else { and reduce the nest in the loop.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:244
+  // Handle PHIs used as arguments in the call-site.
+  for (BasicBlock::iterator BI = Instr->getParent()->begin(),
+                            BE = Instr->getParent()->end();
----------------
range based for loop?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:251
+    unsigned ArgNo = 0;
+    for (CallSite::arg_iterator I = CS.arg_begin(), E = CS.arg_end(); I != E;
+         ++I, ++ArgNo)
----------------
We have the same loop at 3 points now. Would it make sense to add a helper function for that?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:296
+    if (PredBB == OtherPredBB->getSinglePredecessor()) {
+      assert(HeaderBB == nullptr &&
+             "Expect to find only a single header block");
----------------
just !HeaderBB?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:307
+  if (isCondRelevantToAnyCallArgument(Cmp, CS))
+    if (Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_NE)
+      BranchInsts.push_back(PBI);
----------------
maybe have this earlier as a cheap check before we do the more expensive checks?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:316
+  BasicBlock *Parent = Instr->getParent();
+  if (Instr != (Parent->getFirstNonPHI()))
+    return false;
----------------
Unneeded braces


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:319
+
+  for (BasicBlock::iterator BI = Parent->begin(), BE = Parent->end();
+       BI != BE; ++BI) {
----------------
range based for loop?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:352
+  findOrCondRelevantToCallArgument(CS, PredBB2, PredBB1, BranchInsts, HeaderBB);
+  if (BranchInsts.empty() || HeaderBB == nullptr)
+    return false;
----------------
!Header?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:397
+  // instruction.
+  if (Instr != (Instr->getParent()->getFirstNonPHI()))
+    return false;
----------------
Unneeded braces?


https://reviews.llvm.org/D39137