[PATCH] Remove 4,096 loop scale limitation.

[Xinliang David Li]

On Tue, Mar 31, 2015 at 8:01 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Mar 31, at 06:35, Chandler Carruth <chandlerc at gmail.com> wrote:
> >
> > Generally looks good, seems like a nice practical step. Should let
> Duncan check the math before committing.
> >
> >
> > ================
> > Comment at: lib/Analysis/BlockFrequencyInfoImpl.cpp:339-348
> > @@ -344,1 +338,12 @@
> >
> > +  // Infinite loops need special handling. If we give the back edge an
> infinite
> > +  // mass, they may saturate all the other scales in the function down
> to 1,
> > +  // making all the other region temperatures look exactly the same.
> Choose an
> > +  // arbitrary scale to avoid these issues.
> > +  //
> > +  // TODO: An alternate way would be to select a symbolic scale which
> is later
> > +  // replaced to be the maximum of all computed scales plus 1. This
> would
> > +  // appropriately describe the loop as having a large scale, without
> skewing
> > +  // the final frequency computation.
> > +  const Scaled64 InifiniteLoopScale(1, 12);
> > +
> > ----------------
> > Mostly to check that I'm understanding this correctly, this will still
> result in really bizarre behavior where in some cases a non-infinite outer
> loop will be scaled higher than a finite inner loop.
>
> Yes :/.
>
> In practice, we really just have loss of information here; I hope it
> doesn't matter much, but that's what we have.  Some infinite loops
> should be scaled higher than finite loops, others shouldn't; as David
> pointed out, they're not really infinite if we have a profile for
> them.  Consider also that you can have nested "infinite" loops (and the
> inner one can be colder than the entry block of the outer one).
>
> I'm not sure the alternate solution is really any better than what's
> in the patch (or that either is better than a smaller loop scale like
> 64), but I agree it's worth documenting in the code as an option.
>
> If we find that variations on the current and alternate solutions are
> insufficient, another idea is to directly expose whether something is
> infinite to downstream optimizations, something like:
>
>     BlockFrequency getFrequency(BlockT *BB) const;
>     int getLevelOfInfinity(BlockT *BB) const;
>     bool isHotterThan(BlockT *LHS, BlockT *RHS) const {
>       return std::tie(getLevelOfInfinity(LHS), getFrequency(LHS))
>            > std::tie(getLevelOfInfinity(RHS), getFrequency(RHS));
>     }
>
> This still doesn't account for the case where an "infinite" loop is
> colder than the surrounding code though.
>

Longer term, I would still like the infinite loop case to be fixed (when
profile data is available), so we should probably take the simplest
workaround for now :)

David



>
> >
> > Anyways, I would make this a FIXME as this may end up mattering more
> than I would like in practice.
> >
> > ================
> > Comment at: lib/Analysis/BlockFrequencyInfoImpl.cpp:430-431
> > @@ -429,4 +432,1 @@
> >   // room to differentiate small, unequal numbers.
> > -  //
> > -  // TODO: fix issues downstream so that ScalingFactor can be
> > -  // Scaled64(1,64)/Max.
> > ----------------
> > I would probably keep a FIXME here that we should be able to use the
> full 64-bits... Limiting this to 32-bits seems harmless but wasteful.
> >
> > ================
> > Comment at: test/Analysis/BlockFrequencyInfo/bad_input.ll:35
> > @@ -34,2 +34,3 @@
> >
> > -; Check that the loop scale maxes out at 4096, giving 2048 here.
> > +; Check that the infinite loop is arbitrarily scaled to max out a 4096,
> > +; giving 2048 here.
> > ----------------
> > "scaled to max out at" -- missing 't'.
> >
> > http://reviews.llvm.org/D8718
> >
> > EMAIL PREFERENCES
> >  http://reviews.llvm.org/settings/panel/emailpreferences/
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150331/2186edaf/attachment.html>