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

Thu Jan 12 11:58:38 PST 2017

I'm not disagreeing with you. We definitely don't want to drop the metadata
completely.
It's just a question of which assumptions we want to make.

The problem is that the cases where the "normal" assumptions start to break
down are exactly the cases we care about. We don't care whether for a loop
with 1000000 iterations, we estimate the iteration count as 1000000 or
1000001, since we're unlikely to take any action based on that. But it is
probably more important to distinguish loops that, on average, have 0
iterations from the ones that have 1.

So, the question is - what should happen when the average iteration count
of the loop, pre-rotation, is < 1?
And do we have any other interesting cases where this is gives us bad
results? ("Bad" as opposed to "wrong" - that is, probability assignments
that preserve all the right invariants, but lead unrolling/peeling to make
a decision we don't want...)

Dehao may have a somewhat different opinion. :-)

On Thu, Jan 12, 2017 at 11:45 AM, Xin Tong <trent.xin.tong at gmail.com> wrote:

> 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
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170112/43e09831/attachment.html>