[PATCH] D28593: Update loop branch_weight metadata after loop rotation.

[Xin Tong via llvm-commits]

On Thu, Jan 12, 2017 at 11:31 AM, Michael Kuperstein <mkuper at google.com> wrote:
> Dehao and I spent some time staring at this on a whiteboard, and we don't
> have a good conclusion.
> What loop rotation does now looks technically wrong, but it's not clear what
> the right fix is. This is basically the same problem as we have inside loop
> peeling - there isn't really enough information to assign probabilities
> after rotation without knowing the actual distribution.

I have the similar feeling, its difficult to make this precise without
knowing the distribution of the loop trip count. what we have is
merely a summation of all the trip counts for all iterations of the
loop.

>
> We can't just say "If the average trip of the original loop count is X, the
> average trip count of the rotated loop is X-1" and update the edges
> accordingly.

This is not optimal for sure. I think we have 2 options here.
1. We drop the metadata completely, this is not what we want because
it will defeat the purpose of PGO completely.
2. We try to make it as good as possible by making assumptions. Right
now I feel what we have in LoopRotation is wrong as it makes no
attempt to do anything about the should-be-adjusted branch weights.
Having blind branch_weights can be
counter-productive for PGO as optimizations will make decisions based
on those imprecise data.

> As Dehao's example showed, this breaks down, for example, when the average
> trip count is below 1. It's even worse for the first iteration edge - the
> probability is different for "a loop that is always taken 100 times" (in
> which case it's 100/0), and "a loop that is taken 100 times on average -
> half the time it's 0, half the time it's 200" (in which case it's 50/50)...

Yes I considered this case, as I said its not perfect and I feel
assuming the loop has similar # of iterations is a reasonable tradeoff
here. If we can make that assumption in loop peel, i do not see why we
can not during loop rotation.
>
> We'll stare at this some more later.
>
> On Thu, Jan 12, 2017 at 10:37 AM, Xin Tong <trent.xin.tong at gmail.com> wrote:
>>
>> On Thu, Jan 12, 2017 at 10:29 AM, Xin Tong <trent.xin.tong at gmail.com>
>> wrote:
>> > On Thu, Jan 12, 2017 at 10:26 AM, Dehao Chen <danielcdh at gmail.com>
>> > wrote:
>> >>
>> >>
>> >> On Thu, Jan 12, 2017 at 9:26 AM, Michael Kuperstein <mkuper at google.com>
>> >> wrote:
>> >>>
>> >>> I haven't looked at the patch yet, (and I'm also not at the office),
>> >>> but:
>> >>>
>> >>> 1) The new probabilities should be the opposite of the last example
>> >>> Dehao
>> >>> had. That is, the external if should be 1/7, and the one on the
>> >>> backedge
>> >>> 7/28.
>> >>
>> >>
>> >> For the 2nd case, the expected trip count is 0.2, thus it's not
>> >> straight-forward to me how to set this. But if we make the expected
>> >> trip
>> >> count as 5, then looks to me the external if should be 5/1, and the one
>> >> on
>> >> the backedge should be 4/1?
>> >>
>> >> And please make sure that when the input probability is the same, the
>> >> output
>> >> probability should also be the same. E.g., if you annotate the branch
>> >> as (5,
>> >> 1), and (10, 2), after the transformation, the probabilities should be
>> >> equivalent.
>> >>
>> >>>
>> >>> 2) I think this is just conditional probability - cond hasn't changed,
>> >>> but
>> >>> P(cond | cond has been true once) is lower than P(cond). This exactly
>> >>> corresponds to lowering the number of expected iterations by 1, given
>> >>> that
>> >>> we know we already executed one iteration.
>> >>
>> >>
>> >> Seem with this patch, when we compute the estimated trip count, we need
>> >> to
>> >> first check if it's rotated or not: if yes, we use
>> >> (BE.taken/BE.not_taken) +
>> >> 1, otherwise, we use (BE.taken/BE.not_taken). Is that true?
>> >
>>
>> Sorry, Dehao I feel i talked past you, our current way of computing
>> estimated trip count requires loop to be rotated.
>> Yes, in the general case, the trip count of the loop depends on
>> whether the loop is rotated or not as you said.
>>
>> > The way we compute EstimatedTripCount requires the loop to be rotated.
>> > i.e. we only look at the latch block and expect it to be bottom
>> > tested.
>> >>
>> >> Dehao
>> >>
>> >>>
>> >>>
>> >>>
>> >>> On Jan 12, 2017 09:21, "Xin Tong via Phabricator"
>> >>> <reviews at reviews.llvm.org> wrote:
>> >>>
>> >>> trentxintong added a comment.
>> >>>
>> >>> @danielcdh Its the true the cond is not changed, but the inputs to the
>> >>> condition has changed, before you were testing with index variable in
>> >>> the
>> >>> loop header, now u have 2 conditions and the SSA values you use in the
>> >>> conditions have changed, so the probability needs to be adjusted. I
>> >>> will
>> >>> explain more when i get to office.
>> >>>
>> >>>
>> >>> https://reviews.llvm.org/D28593
>> >>>
>> >>>
>> >>>
>> >>>
>> >>
>
>