[PATCH] D14361: Let SelectionDAG start to use probability-based interface to add successors.

[Cong Hou via llvm-commits]

congh marked 2 inline comments as done.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:587
@@ -575,1 +586,3 @@
 
+  // FIXME: Temporarily comment the following code as probabilities are now only
+  // used during instruction lowering, but this interface is called in later
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?
> > > 
> > > Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.
> > > Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?
> > 
> > I think the assertion doesn't hurt and can help us capture exceptions. What is the advantage of using an end iterator? You mean to eliminate some branches?
> > 
> > > 
> > > Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.
> > 
> > This should not happen: we should always guarantee that the weight list is either empty or has the same size of the successor list.
> What I mean is that with the relaxed check (temporary), you can remove the #if 0 ... #endif hack here.
I am afraid that this could not remove #if 0 ... #endif as the root cause is that probability list and successor list may not be synchronized and here we are always removing a successor using an iterator to successor list.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:635
@@ -618,3 +634,3 @@
   // Update its weight instead of adding a duplicate edge.
   if (!Weights.empty()) {
     weight_iterator OldWI = getWeightIterator(OldI);
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > The refactored removeSuccessor can call an internal function
> > > 
> > > removeSuccessor(succ_iterator OldI, succ_iterator NewI) -- when NewI is end(), do not update NewI's weight/prob.
> > > 
> > > replaceSuccessor can then call this method as well.
> > Do you mean replaceSuccessor? If New is not added to the successor list, how to reference it with an iterator?
> I mean make iterator based removeSuccessor interface more general to also take an optional NewI iterator. If NewI is Successors.end(), removeSuccessor behaves the same as the current implementation. If it is a value iterator, removeSuccessor can also be used by replaceSuccessor.
I think it is difficult to let replaceSuccessor() call removeSuccessor(). We have two cases:

1. New is not a successor of MBB. In this case we replace Old with New without removing any successor.

2. New is a successor of MBB. In this case we need to update the weight/probability of New. It seems the current code is more efficient: if we call removeSuccessor() to remove Old, we need to find the weight/probability of it before removing it. We don't want to search the weight/probability twice.


http://reviews.llvm.org/D14361