[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: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,
----------------
Document 'Inst'. Also looks like Inst is not used in the body.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:424
@@ +423,3 @@
+    int IX = PHI->getBasicBlockIndex(OrigBB);
+    if (IX == -1)
+      continue;
----------------
Is this condition possible?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:428
@@ +427,3 @@
+    PHI->addIncoming(V, IndirectCallBB);
+    PHI->setIncomingBlock(IX, DirectCallBB);
+  }
----------------
What is the phi arg value for this new incoming block?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:433
@@ +432,3 @@
+// This method fixes up PHI nodes in BB where BB is the NormalDest of an
+// invoke instruction. In BB, there may be PHIs with incoming block being
+// OrigBB (the MergeBB after if-then-else splitting). After moving the invoke
----------------
invoke --> invoke (indirect)

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:436
@@ +435,3 @@
+// instructions to its own BB, a new incoming edge will be added to the original
+// NormalDstBB from the IndirectCallBB.
+static void fixupPHINodeForNormalDest(Instruction *Inst, BasicBlock *BB,
----------------
Add this description: the OrigBB is still an incoming block for NormalDstBB, but the phi incoming value from this block becomes the new value NewInst 

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


http://reviews.llvm.org/D17864