<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 29, 2017 at 5:12 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="gmail-">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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">danielcdh added a comment.<br>
<span><br>
In <a href="https://reviews.llvm.org/D36052#825318" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3605<wbr>2#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></span><div>If there is no good logical separation to split the buildModuleSimplificationPipel<wbr>ine, it is better to go with option #1.</div></div></div></div></blockquote><div><br></div><div>Chandler, could you sched some lights (I'm asking because Chandler may not want to introduce another flag as suggested in the review of <a href="https://reviews.llvm.org/D36040">https://reviews.llvm.org/D36040</a>)</div><div><br></div><div>I think we should at least have a separate effort to make EarlyFPM not invoked by ThinLTO backend to match the legacy PM's behavior.</div><div><br></div><div>Thanks,</div><div>Dehao</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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/D3605<wbr>2</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>