<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 14, 2017 at 2:01 AM Bozhenov, Nikolai <<a href="mailto:nikolai.bozhenov@intel.com">nikolai.bozhenov@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div class="m_-8895816153576506278WordSection1">
<p class="m_-8895816153576506278MsoPlainText"><a name="m_-8895816153576506278__MailEndCompose">Hi Chandler,<u></u><u></u></a></p></div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-8895816153576506278WordSection1">
<p class="m_-8895816153576506278MsoPlainText"><u></u> <u></u></p>
<p class="m_-8895816153576506278MsoPlainText"><span style="color:#7f7f7f">> This is the *wrong* code review link, and only links to a test update.<u></u><u></u></span></p>
<p class="m_-8895816153576506278MsoPlainText"><u></u> <u></u></p>
</div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-8895816153576506278WordSection1"><p class="m_-8895816153576506278MsoPlainText">The review link is correct. However, Phabricator stopped displaying the patch properly after some point. Here is an “almost fixed” link:<u></u><u></u></p>
<p class="m_-8895816153576506278MsoPlainText"><a href="https://reviews.llvm.org/D34101?vs=106237&id=110556&whitespace=ignore-most#toc" target="_blank">https://reviews.llvm.org/D34101?vs=106237&id=110556&whitespace=ignore-most#toc</a><u></u><u></u></p>
<p class="m_-8895816153576506278MsoPlainText"><u></u> <u></u></p>
<p class="m_-8895816153576506278MsoPlainText"><span style="color:#7f7f7f">> <a href="https://github.com/qemu/qemu/blob/stable-1.0/hw/ide/core.c" target="_blank">https://github.com/qemu/qemu/blob/stable-1.0/hw/ide/core.c</a>
<u></u><u></u></span></p>
<p class="m_-8895816153576506278MsoPlainText"><u></u> <u></u></p>
<p class="m_-8895816153576506278MsoPlainText">Could you suggest what options one should pass to compiler for core.c to reproduce the problem?</p></div></div></blockquote><div>We build QEMU in about 3 or 4 configurations and they all timed out -- I would try a vanilla configure/make build? If that doesn't work I can try to get more specific repro steps.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-8895816153576506278WordSection1"><p class="m_-8895816153576506278MsoPlainText"><u></u><u></u></p>
<p class="m_-8895816153576506278MsoPlainText"><u></u> <u></u></p>
<p class="m_-8895816153576506278MsoPlainText"><span style="color:#7f7f7f">> Where are the numbers?<u></u><u></u></span></p>
<p class="m_-8895816153576506278MsoPlainText"><u></u> <u></u></p>
<p class="m_-8895816153576506278MsoPlainText">I’m not sure what numbers you are asking us for. When the feature was first implemented, we saw some regressions on important benchmarks. That’s why it was disabled with this quite ugly flag. With newer versions of the compiler we cannot
reproduce those regressions, and with the old compiler the regressions were found to be random fluctuations that cannot be addressed in the compiler.</p></div></div></blockquote><div><br></div><div>I mean, the original patch was disabled by a flag because of some regressions, and now the flag is being removed. I'd love to see any discussion of performance implications at all? Did anything in the test suite improve? Was it just the one previously discussed TSVC benhcmark regression that you checked for?</div><div><br></div><div>I guess I'm confused whenever there is a specific mention of performance numbers being better, but then those numbers aren't provided or even really explained or discussed...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-8895816153576506278WordSection1"><p class="m_-8895816153576506278MsoPlainText"><span style="color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#525252">-Nikolai<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Chandler Carruth [mailto:<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>]
<br>
<b>Sent:</b> Monday, August 14, 2017 9:05 AM<br>
<b>To:</b> Bozhenov, Nikolai <<a href="mailto:nikolai.bozhenov@intel.com" target="_blank">nikolai.bozhenov@intel.com</a>>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm] r310583 - [ValueTracking] Enabling ValueTracking patch by default (recommit). Part 2.<u></u><u></u></span></p></div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-8895816153576506278WordSection1">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">Revert landed in r310816. Let me know if you need any help getting the compile issues to reproduce!<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">-Chandler<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Sun, Aug 13, 2017 at 11:53 PM Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<div>
<p class="MsoNormal">On Thu, Aug 10, 2017 at 4:25 AM Nikolai Bozhenov via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal">Author: n.bozhenov<br>
Date: Thu Aug 10 04:24:57 2017<br>
New Revision: 310583<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=310583&view=rev" target="_blank">
http://llvm.org/viewvc/llvm-project?rev=310583&view=rev</a><br>
Log:<br>
[ValueTracking] Enabling ValueTracking patch by default (recommit). Part 2.<br>
<br>
The original patch was an improvement to IR ValueTracking on non-negative<br>
integers. It has been checked in to trunk (D18777, r284022). But was disabled by<br>
default due to performance regressions.<br>
Perf impact has improved. The patch would be enabled by default.<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal">Where are the numbers? I couldn't track anything down because...<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal">Differential Revision: <a href="https://reviews.llvm.org/D34101" target="_blank">
https://reviews.llvm.org/D34101</a><u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">This is the *wrong* code review link, and only links to a test update.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Also:<u></u><u></u></p>
</div>
</div>
</div>
<div>
<div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal">==============================================================================<br>
--- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/ValueTracking.cpp Thu Aug 10 04:24:57 2017<br>
@@ -54,12 +54,6 @@ const unsigned MaxDepth = 6;<br>
static cl::opt<unsigned> DomConditionsMaxUses("dom-conditions-max-uses",<br>
cl::Hidden, cl::init(20));<br>
<br>
-// This optimization is known to cause performance regressions is some cases,<br>
-// keep it under a temporary flag for now.<br>
-static cl::opt<bool><br>
-DontImproveNonNegativePhiBits("dont-improve-non-negative-phi-bits",<br>
- cl::Hidden, cl::init(true));<br>
-<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal">Given that this has already been disabled before (and had to be reverted a few times) it seems very prudent to leave the flag in place and just change the default value at first.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Indeed, we've found it still causes serious compile time issues with code such as:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><a href="https://github.com/qemu/qemu/blob/stable-1.0/hw/ide/core.c" target="_blank">https://github.com/qemu/qemu/blob/stable-1.0/hw/ide/core.c</a>
<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I'm going to revert this for now. If you can't reproduce, we can get a test case together, but building qemu is likely sufficient.<u></u><u></u></p>
</div>
</div>
</div>
</blockquote>
</div>
</div></div></blockquote></div></div>