<div dir="ltr">On 7 October 2013 17:09, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">




<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div><br>
<div class="gmail_quote">On Mon, Oct 7, 2013 at 4:27 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Good thoughts, Chandler. Do I understand you correctly that you agree with Evan’s statement with the modification of s/committed/enabled by default/?</blockquote>





</div><br></div>Essentially, although I tried to be a bit more clear about which buckets different optimizations fall into. For example, in this case, I don't think this change would would have fallen into the bucket where you really need to give some significant heads up prior to enabling by default.</div>





<div class="gmail_extra"><br></div><div class="gmail_extra">I think the problem is this case was very different -- the people making the change *knew* there were a small number of important benchmarks that regressed, and failing to address that prior to enabling or committing something is quite different.</div>




</div></blockquote><div><br></div><div>In order for this optimization to fire at -O2, you need to not use local variables and instead write them as global variables and only use them from a single function. A single source-level function, because this optimization occurs before inlining.<br>


</div>
<div><br></div><div>I wasn't aware of a small number of important tests. I'd heard from somebody once upon a time that there's a single test in SPEC which did this, but that's all I know. I don't know which benchmark in SPEC, or whether it exists in the current version of SPEC. I judged, based on the workings of the optz'n, that it was a silly optimization and unlikely to be very important, doubly so considering that it risks miscompiling if it ever fires-- a strong suggestion that it never fires.</div>


<div><br></div><div>It wasn't until Evan's complaint that I learned that this pattern exists in two tests in the nightly test suite, which was a real shock to me. But please don't conflate that with what we knew before it was committed.</div>

<div><br></div><div>I do know this micro-opt miscompiles. Perhaps it only got the performance it did because it went down wrong code paths, ultimately emitting the right text following wrong reasoning to get there. Unless someone proves otherwise, why should we be bound by the performance this test used to get?</div>

<div><br></div><div>Nick<br></div><div><br></div></div></div></div>