[PATCH] D39137: Add CallSiteSplitting pass

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:473
+  if (OptLevel > 2)
+    MPM.add(createCallSiteSplittingPass());
+
----------------
should this be moved after CFG simplification pass ? The latter may undo the splitting.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:169
+
+static bool splitCallSite(CallSite CS, BasicBlock *PredBB1, BasicBlock *PredBB2,
+                          Instruction *CallInst1, Instruction *CallInst2) {
----------------
Document this method -- describe parameters etc.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:321
+  // instruction.
+  if (Instr != (Instr->getParent()->getFirstNonPHI()))
+    return false;
----------------
move the check to the caller.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:326
+  // without too much effort.
+  if (!isa<CallInst>(Instr))
+    return false;
----------------
move the check to caller.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:365
+  if (!splitCallSite(CS, PredBB1, PredBB2, CallInst1, CallInst2)) {
+    if (CallInst1)
+      CallInst1->deleteValue();
----------------
if splitCallSite returns false, how can CallInst1 be non-null?


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:375
+
+static bool processToSplitCallSites(Function &F) {
+  bool Changed = false;
----------------
nit: shorten the name to just 'doCallSiteSplitting'


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:377
+  bool Changed = false;
+  for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE;) {
+    BasicBlock *BB = &*BI++;
----------------
use range for loop: for (BasicBlock &BB: F) 


================
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++;
----------------
use range loop.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:382
+      CallSite CS(cast<Value>(I));
+      if (!CS)
+        continue;
----------------
Skip intrinsicInst. Or if there are folding opportunities, add test cases to cover it.


================
Comment at: test/Transforms/CallSiteSplitting/callsite-split.ll:1
+; RUN: opt < %s -callsite-splitting -inline -instcombine -jump-threading -S | FileCheck %s
+
----------------
add test for new PM.


https://reviews.llvm.org/D39137