[PATCH] Remove 4,096 loop scale limitation.

Duncan P. N. Exon Smith dexonsmith at apple.com
Tue Mar 31 06:59:39 PDT 2015


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

Sorry for the repeat comments; I didn't notice Chandler's review
until I sent mine :).

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





More information about the llvm-commits mailing list