<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 29, 2017 at 4:39 PM, Dehao Chen via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">danielcdh added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D36052#825318" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36052#825318</a>, @davidxl wrote:<br>
<br>
> Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread.  This one should be left as SamplePGO only change.<br>
<br>
<br>
</span>The tricky part is that PGOOptions was not passed in the ThinLTO backend before this patch. But after this patch, it will be passed in. However, in buildModuleSimplificationPipel<wbr>ine, it cannot know if this is the ThinLTO backend. As a result, we cannot disable instrumentation/annotation pass in there.<br>
<br>
The potential solution could be:<br>
<br>
- add another boolean value in buildModuleSimplificationPipel<wbr>ine to indicate it's ThinLTO backend<br>
- refactor the code from the beginning of buildModuleSimplificationPipel<wbr>ine to the PGOOpt handling to a separate function, and call it separately in the caller of buildModuleSimplificationPipel<wbr>ine.<br></blockquote><div><br></div><div><br></div><div>If there is no good logical separation to split the buildModuleSimplificationPipeline, it is better to go with option #1.</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Any suggestions on how to move forward?<br>
<br>
Thanks,<br>
Dehao<br>
<br>
<br>
<a href="https://reviews.llvm.org/D36052" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36052</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>