<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 15, 2016 at 6:21 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.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="">On Fri, Jan 15, 2016 at 6:09 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
> silvas added inline comments.<br>
><br>
> ================<br>
> Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:216<br>
> @@ +215,3 @@<br>
> +    MPM.add(createGVNPass(DisableGVNLoadPRE));  // Remove redundancies<br>
> +    addExtensionsToPM(EP_Peephole, MPM);<br>
> +  }<br>
> ----------------<br>
> Let's drop the pre-instrumentation passes for now and leave a TODO. We will want to have a focused discussion on what to put here.<br>
><br>
> ================<br>
> Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:281<br>
> @@ -239,1 +280,3 @@<br>
><br>
> +  addPGOInstrPasses(MPM);<br>
> +<br>
> ----------------<br>
>> Yes, and there was not consensus in the RFC for the pre-inlining. Instead, the approach suggested was to work on independent components which seem non-controversial and have a separate focused discussion about pre-inlining and other interactions.<br>
><br>
> Agreed. We will definitely want to have a focused discussion with real-world performance measurements when doing pass pipeline tuning (I have a big chunk of time allocated for characterizing and tuning the new IR-level instrumentation stuff and am looking forward to participating!). For the moment, just wiring it up and leaving a TODO for the pre-instrumentation passes is probably the right incremental step.<br>
><br>
<br>
</span>My suggestion is to leave the pipeline changes in but not off by<br>
default in some way. This allows much easier experiments with options<br>
flipped on and off.<br>
<br>
For serious performance tuning, I strongly recommend doing so<br>
(extensively) only after PGO data is hooked up with inliner. Without<br>
that in place, the results will be misleading.<br></blockquote><div><br></div><div>Not for analyzing the performance benefits for the instrumented build. Leaving in the pipeline changes as an on/off option is misleading, because there are many more possibilities than just what you have there now. Testing on/off only what you have there now (and not other variations) is not particularly interesting (and may be actively detrimental if it suggests that there are only two configurations to test).</div><div><br></div><div>As part of a more focused discussion we can think about what is interesting to test and add flags to support that. But adding on/off support for a single arbitrary configuration is not particularly interesting.</div><div><br></div><div>-- Sean Silva</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
> (I just got back from vacation and am still working on my email backlog, but I should be back on my feet next week and the new IR-level instrumentation stuff is my top priority at the moment)<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D15828" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15828</a><br>
><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>