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

[Rong Xu via llvm-commits]

xur marked 3 inline comments as done.

================
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) {
----------------
davidxl wrote:
> 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.
Note the condition "if (DirectCallBB)"
I only change the phi operand points to DirectcallBB when it's not a nullptr.

For normalDest, the DirectcallBB is set to nullptr in the caller.
This also answers your question about the call's nullptr argument.



================
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);
----------------
davidxl wrote:
> nullptr ? It does not look correct. It shold be the DirectcallInst.
see the reply in fixupPHINode.

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

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:526
@@ +525,3 @@
+  BasicBlock *PHIBB;
+  if (InvokeInst *II = dyn_cast<InvokeInst>(CallResult))
+    RetValBB = II->getNormalDest();
----------------
davidxl wrote:
> 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.
The phi is not inserted in the mergeBB. It's in the BB the joins mergeBB and original NormalDest. We now have one return value from the original indirect invoke and one from new direct invoke. A phi is needed in the join point.


http://reviews.llvm.org/D17864