<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 31, 2015 at 8:01 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2015 Mar 31, at 06:35, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
><br>
</span><span class="">> Generally looks good, seems like a nice practical step. Should let Duncan check the math before committing.<br>
><br>
><br>
> ================<br>
> Comment at: lib/Analysis/BlockFrequencyInfoImpl.cpp:339-348<br>
> @@ -344,1 +338,12 @@<br>
><br>
> +  // Infinite loops need special handling. If we give the back edge an infinite<br>
> +  // mass, they may saturate all the other scales in the function down to 1,<br>
> +  // making all the other region temperatures look exactly the same. Choose an<br>
> +  // arbitrary scale to avoid these issues.<br>
> +  //<br>
> +  // TODO: An alternate way would be to select a symbolic scale which is later<br>
> +  // replaced to be the maximum of all computed scales plus 1. This would<br>
> +  // appropriately describe the loop as having a large scale, without skewing<br>
> +  // the final frequency computation.<br>
> +  const Scaled64 InifiniteLoopScale(1, 12);<br>
> +<br>
> ----------------<br>
> 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.<br>
<br>
</span>Yes :/.<br>
<br>
In practice, we really just have loss of information here; I hope it<br>
doesn't matter much, but that's what we have.  Some infinite loops<br>
should be scaled higher than finite loops, others shouldn't; as David<br>
pointed out, they're not really infinite if we have a profile for<br>
them.  Consider also that you can have nested "infinite" loops (and the<br>
inner one can be colder than the entry block of the outer one).<br>
<br>
I'm not sure the alternate solution is really any better than what's<br>
in the patch (or that either is better than a smaller loop scale like<br>
64), but I agree it's worth documenting in the code as an option.<br>
<br>
If we find that variations on the current and alternate solutions are<br>
insufficient, another idea is to directly expose whether something is<br>
infinite to downstream optimizations, something like:<br>
<br>
    BlockFrequency getFrequency(BlockT *BB) const;<br>
    int getLevelOfInfinity(BlockT *BB) const;<br>
    bool isHotterThan(BlockT *LHS, BlockT *RHS) const {<br>
      return std::tie(getLevelOfInfinity(LHS), getFrequency(LHS))<br>
           > std::tie(getLevelOfInfinity(RHS), getFrequency(RHS));<br>
    }<br>
<br>
This still doesn't account for the case where an "infinite" loop is<br>
colder than the surrounding code though.<br></blockquote><div><br></div><div>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 :)</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> Anyways, I would make this a FIXME as this may end up mattering more than I would like in practice.<br>
><br>
> ================<br>
> Comment at: lib/Analysis/BlockFrequencyInfoImpl.cpp:430-431<br>
> @@ -429,4 +432,1 @@<br>
>   // room to differentiate small, unequal numbers.<br>
> -  //<br>
> -  // TODO: fix issues downstream so that ScalingFactor can be<br>
> -  // Scaled64(1,64)/Max.<br>
> ----------------<br>
> 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.<br>
><br>
> ================<br>
> Comment at: test/Analysis/BlockFrequencyInfo/bad_input.ll:35<br>
> @@ -34,2 +34,3 @@<br>
><br>
> -; Check that the loop scale maxes out at 4096, giving 2048 here.<br>
> +; Check that the infinite loop is arbitrarily scaled to max out a 4096,<br>
> +; giving 2048 here.<br>
> ----------------<br>
> "scaled to max out at" -- missing 't'.<br>
><br>
> <a href="http://reviews.llvm.org/D8718" target="_blank">http://reviews.llvm.org/D8718</a><br>
><br>
> EMAIL PREFERENCES<br>
>  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>