<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 31, 2015 at 6:35 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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></blockquote><div><br></div><div>you mean 'higher than infinite inner loop' here?</div><div><br></div><div>No it does not matter as we are approximating here.  Static 'infinite' loop does not mean it will have high dynamic iteration count or vise versa.</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>