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

[Cong Hou via llvm-commits]

On Tue, Nov 17, 2015 at 3:50 PM, Xinliang David Li <davidxl at google.com> wrote:
>
>
> 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?

There is an assert in getProbabilityIterator() checking if the
probability list and successor list have the same size. So I think we
don't want to remove those assertions too. The prob iterator is got by
calculating the offset of the successor iterator. I feel it is unsafe
to return end() if those two lists are not synchronized.

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

Note that there are two additional statements in replaceSuccessor()
comparing to removeSuccessor(): we need to update the
weight/probability of NewI from OldI. Even we replace this part with
removeSuccessor(), we still need those branches to check if
weight/probability lists are empty, get the weight/probability
iterators and then update them. I will update the code.

>
> David
>
>
>
>>
>> http://reviews.llvm.org/D14361
>>
>>
>>
>