[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:408
@@ +407,3 @@
+
+// Fix the PHI node in the basic block BB. We have the PHIs from
+// OrigBB and we need to duplicate them for the IndirectCall BB.
----------------
Fix the comments.

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 if-then-else splitting, OrigBB is no longer the predecessor block of BB. Instead two new predecessors are added: IndirectCallBB and DirectCallBB, so the PHI nodes's incoming BBs need to be fixed up accordingly.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:412
@@ +411,3 @@
+static void fixupPHINode(Instruction *Inst, Instruction *NewInst,
+                         BasicBlock *BB, BasicBlock *OrigBB,
+                         BasicBlock *IndirectCallBB, BasicBlock *DirectCallBB) {
----------------
This method is correct only for unwind/eh dest BB for an invoke instruction. It is not right for the normalDest.

For normalDest BB, the PHI node's predecessor BB remain unchanged -- OrigBB (i.e. MergeBB), but the value of the PHI incoming value may change into new PHI or cast inst inserted into the MergeBB.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:428
@@ +427,3 @@
+
+    PHI->addIncoming(V, IndirectCallBB);
+    if (DirectCallBB)
----------------
Add assert that V is defined in a BB that dominates IndirectCallBB

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:478
@@ +477,3 @@
+
+  // Clean the value profile data.
+  NewInst->setMetadata(LLVMContext::MD_prof, 0);
----------------
Clean --> Clear

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:497
@@ +496,3 @@
+
+// After creating IF_Then_Else, the PHIs for NormalDest and UnwindDest point to
+// the mergeBB (which is the new NormalDest for direct-call. We need to
----------------
This comment needs lots of cleanup.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:502
@@ +501,3 @@
+// don't need to DirectCallBB for updating the PHIs for NormalDest.
+static void fixupInvokeInst(Instruction *Inst, Instruction *DirectCallInst,
+                            Instruction *DirectCallResult) {
----------------
Since this function body computes lots of values already available in the caller context, suggest removing it and inline it into the caller.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:511
@@ +510,3 @@
+  BasicBlock *MergeBB = dyn_cast<InvokeInst>(DirectCallInst)->getNormalDest();
+  fixupPHINode(Inst, nullptr, II->getUnwindDest(), MergeBB, IndirectCallBB,
+               DirectCallBB);
----------------
nullptr ? It does not look correct. It shold be the DirectcallInst.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:513
@@ +512,3 @@
+               DirectCallBB);
+  fixupPHINode(Inst, DirectCallResult, II->getNormalDest(), MergeBB,
+               IndirectCallBB, nullptr);
----------------
see comment in fixupPHINode method.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:526
@@ +525,3 @@
+  BasicBlock *PHIBB;
+  if (InvokeInst *II = dyn_cast<InvokeInst>(CallResult))
+    RetValBB = II->getNormalDest();
----------------
For Invoke, since the cast is inserted in the original BB (mergeBB) -- i.e., the new NormalDest, it post-dominates the call return values from both predecessors so there seems to be no need for phi insertion -- unless you split the another new BB between DirectBB and MergeBB.


http://reviews.llvm.org/D17864