[PATCH] D17864: [PGO] Promote indirect calls to conditional direct calls with value-profile

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:387
@@ +386,3 @@
+
+  // Add jump from Merge BB to the NormalDest.
+  BranchInst::Create(II->getNormalDest(), *MergeBB);
----------------
Document that this is needed for the newly created direct call (invoke) -- as its normal Dst will be fixed up to MergeBB

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:408
@@ +407,3 @@
+
+// This method fixes up PHI nodes in block BB. In BB, there may be PHIs with
+// incoming block being OrigBB (the MergeBB after block splitting). After
----------------
.. in BB (where BB can either be the original NormalDst BB or the unwind BB of the indirect call/invoke instruction).

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:410
@@ +409,3 @@
+// incoming block being OrigBB (the MergeBB after block splitting). After
+// if-then-else splitting, OrigBB is no longer the predecessor block of BB.
+// Instead two new predecessors are added: IndirectCallBB and DirectCallBB,
----------------
After if-then-else splitting, if BB is the unwindBB, OrigBB will no longer be the pre...

If BB is the Original NormalDstBB, a new incoming edge will be added to the original NormalDstBB from the IndirectCallBB.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:413
@@ +412,3 @@
+// so the PHI nodes's incoming BBs need to be fixed up accordingly.
+static void fixupPHINode(Instruction *Inst, Instruction *NewInst,
+                         BasicBlock *BB, BasicBlock *OrigBB,
----------------
I suggest split this method into two fixPHINodesForNormDest and fixPHINodesForUnwind -- for better readability.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:458
@@ +457,3 @@
+
+// Create a DirectCall instruction in the DirectCallBB.
+static Instruction *createDirectCallInst(const Instruction *Inst,
----------------
Document parameters. For instance, for mergeBB, document that it is the mergeBB (bottom BB of the if-then-else-diamond) after indirect call promotion. For Invoke instruction, the  edges from directBB and IndirectBB to mergedBB are removed before this call (during createIfThenElse).   

Also document the CFG effect of this method: after the call, the direct invokes normal destination will be redirected to MergeBB.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:462
@@ +461,3 @@
+                                         BasicBlock *DirectCallBB,
+                                         BasicBlock *NormalDest) {
+  Instruction *NewInst = Inst->clone();
----------------
NormalDest --> MergeBB

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:473
@@ +472,3 @@
+    II->mutateFunctionType(DirectCallee->getFunctionType());
+    II->setNormalDest(NormalDest);
+  }
----------------
Add a comment lie: 

direct Invoke's normal destinaion is fixed up to mergeBB. MergeBB is the place where return cast is inserted. Also since indirectCallBB does not have an edge to MergeBB, there is no need to insert new PHIs into MergeBB.


http://reviews.llvm.org/D17864