[PATCH] D15259: Normalize MBB's successors' probabilities in several locations.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:464
@@ -463,2 +463,3 @@
   /// Predecessors list of Succ is automatically updated.
-  void removeSuccessor(MachineBasicBlock *Succ);
+  /// If NormalizeSuccProbs is true, then normalize successors' probabilities
+  /// after the successor is removed.
----------------
Can you add some comment about the guidelines when to use the normalize arg? For instance when the removal is the final one.

================
Comment at: lib/CodeGen/IfConversion.cpp:1262
@@ -1261,2 +1261,3 @@
     TII->InsertBranch(*BBI.BB, CvtBBI->FalseBB, nullptr, RevCond, dl);
     BBI.BB->addSuccessor(CvtBBI->FalseBB, NewFalse);
+    BBI.BB->normalizeSuccProbs();
----------------
Is it better to add the same 'normalize' parameter to the addSuccessor method?

A related -- for addSuccessor, what is the guideline to normalize? How can we tell if the normalization actually masks a bug in the updater?

================
Comment at: lib/CodeGen/TailDuplication.cpp:754
@@ -753,2 +753,3 @@
       PredBB->addSuccessor(NewTarget, Prob);
+    PredBB->normalizeSuccProbs();
 
----------------
This one seems unnecessary.

================
Comment at: lib/CodeGen/TailDuplication.cpp:862
@@ -860,2 +861,3 @@
            E = TailBB->succ_end(); I != E; ++I)
       PredBB->addSuccessor(*I, MBPI->getEdgeProbability(TailBB, I));
+    PredBB->normalizeSuccProbs();
----------------
The normalization seems redundant -- See assertion before: assert(PredB->succ_empty()) ..


If the assertion is not true, the way prob is updated is not correct. It should do this:
OldProb = getEdgeProb(PredBB, PredBB->succ_begin());

for (.... ) {
   PredBB->addSuccessor (*I, getEdgeProb(TailBB,I)*OldProb)
}

Normalization is not needed either.


http://reviews.llvm.org/D15259