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

[Xinliang David Li via llvm-commits]

On Tue, Nov 17, 2015 at 3:37 PM, Cong Hou <congh at google.com> wrote:

> 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.
>
>
In this case, would the getProbabilityIterator return end/null iterator,
which the caller can skip?


> ================
> 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.
>
>
>
I am not suggesting replacing replaceSuccessor with removeSuccessor, but
just the part that does 1) update weight list, 2) update prob list, and 3)
erase oldI from Successors.

When the code reaches this part, NewI should be valid -- see there is an
early return when NewI replaces OldI.  There is also no efficiency issue I
see -- OldI and NewI are already computed.

David




> http://reviews.llvm.org/D14361
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151117/637109bd/attachment.html>