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

[Cong Hou via llvm-commits]

congh 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.
----------------
davidxl wrote:
> Can you add some comment about the guidelines when to use the normalize arg? For instance when the removal is the final one.
I have added some comments on normalizeSuccProbs(). This is usually done when the current update on the MBB is done. There are three effects of using this interface: 1. Incorrect. 2. Necessary. 3. Redundant. We should forbid the first case and try to always do it if it is necessary.

================
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();
----------------
davidxl wrote:
> 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?
As the user already provided a probability when calling addSuccessor(), I think adding the 'normalize' parameter to it is unnecessary. Actually in most cases we don't have to do the normalization. 

The time to do normalization is the update is done and no more update is coming to this MBB. It is difficult to detect if a normalization is correct, or may be expensive to do this. It is the user's responsibility to keep its usage correct.

================
Comment at: lib/CodeGen/TailDuplication.cpp:754
@@ -753,2 +753,3 @@
       PredBB->addSuccessor(NewTarget, Prob);
+    PredBB->normalizeSuccProbs();
 
----------------
davidxl wrote:
> This one seems unnecessary.
Agree. Thanks for pointing it out!.

================
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();
----------------
davidxl wrote:
> 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.
You are right. The normalization is redundant here.


http://reviews.llvm.org/D15259