[PATCH] Remove 4,096 loop scale limitation.

[Duncan P. N. Exon Smith]

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

> 
> 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/
> 
>