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

[David Li via llvm-commits]

davidxl added inline comments.

================
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
----------------
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.

================
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);
----------------
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.


http://reviews.llvm.org/D14361