<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 23, 2016 at 8:23 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Jake and I have been integrating IRPGO on PS4, and we've identified 3 remaining work items.</div></blockquote><div><br></div><div>Sean, thanks for the write up. It matches very well with what we think as well. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><br></div><div>- Driver changes</div><div><br></div><div>We'd like to make IRPGO the default on PS4. We also think that it would be beneficial to make IRPGO the default PGO on all platforms (coverage would continue to use FE instr as it does currently, of course). In previous conversations (e.g. <a href="http://reviews.llvm.org/D15829" target="_blank">http://reviews.llvm.org/D15829</a>) it has come up that Apple have requirements that would prevent them from moving to IRPGO as the default PGO, at least without a deprecation period of one or two releases.</div></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I'd like to get consensus on a path forward.</div><div>As a point of discussion, how about we make IRPGO the default on all platforms except Apple platforms. I really don't like fragmenting things like this (e.g. if a third-party tests "clang's" PGO they will get something different depending on the platform), but I don't see another way given Apple's constraints.</div><div><br></div></div></blockquote><div><br></div><div>I'd like to see IRPGO to be the default as well, but the first thing we need is a driver level option to make the switch (prof-gen) -- currently we rely on -Xclang option to switch between two modes, which is less than ideal. </div><div><br></div><div>If the concern from Apple is that the old profile still need to work, then this is problem already solved. The reason is that -fprofile-instr-use can automatically detect the type of the profile and switch the mode.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div><br></div><div>- Pre-instrumentation passes</div><div><br></div><div>Pre-instrumentation optimization has been critical for reducing the overhead of PGO for the PS4 games we tested (as expected). However, in our measurements (and we are glad to provide more info) the main benefit was inlining (also as expected). A simple pass of inlining at threshold 100 appeared to give all the benefits. Even inlining at threshold 0 gave almost all the benefits. For example, the passes initially proposed in <a href="http://reviews.llvm.org/D15828" target="_blank">http://reviews.llvm.org/D15828</a> did not improve over just inlining with threshold 100.</div><div><br></div><div>(due to PR27299 we also need to add simplifycfg after inlining to clean up, but this doesn't affect the instrumentation overhead in our measurements)</div><div><br></div><div>Bottom line: for our use cases, inlining does all the work, but we're not opposed to having more passes, which might be beneficial for non-game workloads (which is most code).</div><div><br></div><div><br></div></div></blockquote><div><br></div><div>Yes, Rong is re-collecting performance data before submitting the patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>- Warnings</div><div><br></div><div>We identified 3 classes of issues which manifest as spammy warnings when applying profile data with IRPGO (these affect FEPGO also I believe, but we looked in depth at IRPGO):</div><div><br></div><div>1. The main concerning one is that getPGOFuncName mangles the filename into the counter name. This causes us to get instrprof_error::unknown_function when the pgo-use build is done in a different build directory from the training build (which is a reasonable thing to support). In this situation, PGO data is useless for all `static` functions (and as a byproduct results in a huge volume of warnings).</div></div></blockquote><div><br></div><div>This can be enhanced with an user option to override the behavior. Can you help filing a tracking bug? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>2. In different TU's, pre-instr inlining might make different inlining decisions (for example, different functions may be available for inlining), causing hash mismatch errors (instrprof_error::hash_mismatch). In building a large game, we only saw 8 instance of this, so it is not as severe as 1, but would be good to fix.</div><div><br></div></div></blockquote><div><br></div><div>Rong has a patch addressing that -- will submit after cleanup pass change is done.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>3. A .cpp file may be compiled and put into an archive, but then not selected by the linker and will therefore not result in a counter in the profraw. When compiling this file with pgo-use, instrprof_error::unknown_function will result and a warning will be emitted.</div></div></blockquote><div><br></div><div>yes -- this is a common problem to other compilers as well. </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Case 1 can be fixed using a function hash or other unique identifier instead of a file path. David, in D20195 you mentioned that Rong was working on a patch that would fix 2; we are looking forward to that.</div><div><br></div></div></blockquote><div><br></div><div>Right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>For 3, I unfortunately do not know of any solution. I don't think there is a way for us to make this warning reliable in the face of this circumstance. So my conclusion is that instrprof_error::unknown_function at least must be defaulted to off unfortunately.</div></div></blockquote><div><br></div><div>yes, this can be annoying. If the warnings can be buffered, then the compiler can check if this is due to missing profile for the whole file and can reduce the warnings into one single warning (source file has no profile data).  Making it off by default sounds fine to me too if it is too noisy.</div><div><br></div><div>thanks,</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"><div dir="ltr"><span class="HOEnZb"><font color="#888888"><div><div><br></div><div>-- Sean Silva</div></div></font></span></div>
</blockquote></div><br></div></div>