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

[Rong Xu via llvm-commits]

xur marked 2 inline comments as done.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:415
@@ +414,3 @@
+// so the PHI node's incoming BBs need to be fixed up accordingly.
+static void fixupPHINodeForUnwind(Instruction *Inst, BasicBlock *BB,
+                                  BasicBlock *OrigBB,
----------------
davidxl wrote:
> Document 'Inst'. Also looks like Inst is not used in the body.
you are right. Inst parameter is only used to check the invoke return value. I assume the UnwindDest will not have a phi using the return value. 

I'll remove Inst from the parameter list.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:424
@@ +423,3 @@
+    int IX = PHI->getBasicBlockIndex(OrigBB);
+    if (IX == -1)
+      continue;
----------------
davidxl wrote:
> Is this condition possible?
why not? The operand can come from a basic block other than OrigBB:
like bb1 (def v_1) --> bb2(invoke) --> bb
       bb2(def v_2) --> bb
bb can have a phi(v_1, v_2) in bb.
The incoming block for the phi with ix 0 is bb1, rather bb2, right?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:428
@@ +427,3 @@
+    PHI->addIncoming(V, IndirectCallBB);
+    PHI->setIncomingBlock(IX, DirectCallBB);
+  }
----------------
davidxl wrote:
> What is the phi arg value for this new incoming block?
it's the same value as the one from OriginBB: in the two statements, the first only dup the value, and second one rest the incoming bb to DirectCallBB.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:447
@@ +446,3 @@
+    if (IX == -1)
+      continue;
+    Value *V = PHI->getIncomingValue(IX);
----------------
davidxl wrote:
> Is IX == -1 possible?
see earlier reply.


http://reviews.llvm.org/D17864