[PATCH] D39137: Add CallSiteSplitting pass

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added inline comments.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:25
+// to :
+//   if (!ptr)
+//     callee(nonnull ptr)  // set non-null attribute in the argument
----------------
I believe this should be:
  //  if (!ptr)
  //    callee(null)         // set the known constant value
  //  else if (c)
  //    callee(nonnull ptr)  // set non-null attribute in the argument



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:113
+    BranchInst *PBI = BranchInsts[I];
+    assert(PBI->isConditional());
+    ICmpInst *Cmp = cast<ICmpInst>(PBI->getCondition());
----------------
  assert(PBI->isConditional() && "Expected predecessor block to end in a conditional branch.");


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:275
+       ++I, ++ArgNo) {
+    // Don't consider arguments that are already known non-null.
+    if (CS.paramHasAttr(ArgNo, Attribute::NonNull))
----------------
You might consider skipping Constant arguments before checking for the NonNull attribute.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:302
+  Value *Cond = PBI->getCondition();
+  if (match(Cond, m_ICmp(Pred, m_Value(), m_Constant()))) {
+    ICmpInst *Cmp = cast<ICmpInst>(Cond);
----------------
Improve indent:

  if (match(Cond, m_ICmp(Pred, m_Value(), m_Constant())))
    return;
  ICmpInst *Cmp = cast<ICmpInst>(Cond);
  if (isCondRelevantToAnyCallArgument(Cmp, CS))
    if (Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_NE)
      BranchInsts.push_back(PBI);



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:314
+  Instruction *Instr = CS.getInstruction();
+  if (Instr != (Instr->getParent()->getFirstNonPHI()))
+    return false;
----------------
  BasicBlock *Parent = Instr->getParent();
  if (Instr != (Parent->getFirstNonPHI()))
    return false;

  for (BasicBlock::iterator BI = Parent->begin(), BE = Parent->end();


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:425
+  for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE;) {
+    BasicBlock *BB = &*BI++;
+    for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
----------------
I think you can just use a reference to BB:

    BasicBlock &BB = *BI++;
    for (BasicBlock::iterator II = BB.begin(), IE = BB.end(); II != IE;) {


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:479
+
+  if (!Changed)
+    return PreservedAnalyses::all();
----------------
You can eliminate the Changed boolean by:

  if (!doCallSiteSplitting(F, TLI))
    return PreservedAnalyses::all();


https://reviews.llvm.org/D39137