[PATCH] D39137: Add CallSiteSplitting pass

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:546
+
+  if (Level == O3)
+    EarlyFPM.addPass(CallSiteSplittingPass());
----------------
fhahn wrote:
> Is there any reason you use Level == 3 here and Level > 2 in lib/Transforms/IPO/PassManagerBuilder.cpp?
Level in lib/Passes/PassBuilder.cpp  is an enum defined : 
enum OptimizationLevel {
    O0,
    O1,
    O2,
    O3,
    Os,
    Oz
  };

while, OptLevel in lib/Transforms/IPO/PassManagerBuilder.cpp is defined : 

  /// The Optimization Level - Specify the basic optimization level.
  ///    0 = -O0, 1 = -O1, 2 = -O2, 3 = -O3
  unsigned OptLevel;




================
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)
----------------
fhahn wrote:
> We have the same loop at 3 points now. Would it make sense to add a helper function for that?
Changed to a range-based  loop, but didn't add helper function because the purpose of each loop slightly different: first one adds an attribute, and the second one set argument value for one call-site,  and the third one set arguments for two call-sites.  IMHO,  iterating for each case doesn't make a big confusion.   


================
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);
----------------
fhahn wrote:
> maybe have this earlier as a cheap check before we do the more expensive checks?
Agree.


https://reviews.llvm.org/D39137