<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 3, 2017 at 7:27 PM, Teresa Johnson <span dir="ltr"><<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Fri, Feb 3, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 3, 2017 at 4:51 PM, Rong Xu <span dir="ltr"><<a href="mailto:xur@google.com" target="_blank">xur@google.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">ThinLTO is fact coupled with PGO naming scheme as of now. It needs to have the same naming scheme to import the right indirect-call function. <div><br></div><div>We can move this options to InstrProf.cpp, but this will break the indirect-call-promotion in thin-lto mode.</div></div></blockquote><div><br></div></span><div>It is already broken with -static-func-full-module-name.<wbr>  The new option's main purpose is to make prof-gen and use consistent, so it belongs to PGO only.</div></div></div></div></blockquote><div><br></div></span><div>But if the option is enabled in some fashion (like the current option is enabled), we will lose ThinLTO indirect call promotion to static functions since the name won't match. </div></div></div></div></blockquote><div><br></div><div>This is referring to the second icall promotion pass right? There is no reason such name mismatch should happen. If I remember correctly, the indirect call target's IC value is MD5 sum of the PGOFuncName.  In FE compilation of the PGO use phase, PGO annotator will annotate the static function with the PGOFuncName meta data.  As long as the striping is applied the same way in Gen and Use phase, the PGO name will match.</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 class="gmail_extra"><div class="gmail_quote"><div>Note we don't actually want to do any path prefix stripping in ThinLTO mode, since otherwise we will hit other issues (conflicts in the index). So ideally we won't have these options enabled at the same time as ThinLTO, but is there a way to detect and enforce that?</div></div></div></div></blockquote><div><br></div><div>The stripping is off by default.</div><div><br></div><div>David </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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'm not sure if I understand the reconstruction method. How can this reconstruction affect the importing in thin-lto?</div></div></blockquote><div><br></div></span><div>The issue is that after static promotion (static func becomes global), the promoted/raw name needs to be used for target lookup -- this is the reason why the static promotion currently needs to use the exact same scheme as PGO.  </div></div></div></div></blockquote><div><br></div></span><div>Which static promotion are you referring to? ThinLTO does not use the PGO name (aka global identifier) for static promotion. Only as the key to the combined index. It is this key that needs to match the PGO name for ThinLTO indirect call promotion to work.</div><div><br></div><div>Teresa</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>What I suggest is that PGO code should recognize the promoted symbol and use its original name+linkage or use the PGO name metadata you introduced.</div><span class="m_5575846267248064601HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="m_5575846267248064601m_-8338666240948728265h5"><div><br></div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 3, 2017 at 4:16 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"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 3, 2017 at 4:00 PM, Teresa Johnson 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">tejohnson added inline comments.<br>
<br>
<br>
================<br>
Comment at: lib/IR/Globals.cpp:159<br>
<span>+    else {<br>
+      if (StaticFuncStripDirNamePrefix != 0)<br>
+        FileName = stripDirPrefix(FileName, StaticFuncStripDirNamePrefix);<br>
</span>----------------<br>
<span>davidxl wrote:<br>
> getGlobalIdentifier is a low level interface. Move this into getPGOFuncName<br>
</span>Rong and I discussed this - putting it here ensures the same naming scheme is used for PGO and for the ThinLTO index. Although for ThinLTO we will likely not want this enabled. I'm not sure if there is a good way to enforce that.<br>
<br></blockquote><div><br></div></span><div>Ideally we should not couple ThinLTO naming scheme with PGO lookup name scheme -- PGO code should be able to 'reconstruct' the original static function name and then use getPGOFuncName to form the lookup name -- but that is for later.</div><div><br></div><div>For now, it is better to isolate this internal option to the domain of PGO only. </div><div><br></div><div>David</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D29512" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2951<wbr>2</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></div></div>
</blockquote></span></div><br></div></div>
</blockquote></span></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div class="m_5575846267248064601gmail_signature" data-smartmail="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> <a href="tel:(408)%20460-2413" value="+14084602413" target="_blank">408-460-2413</a></td></tr></tbody></table></span></div>
</font></span></div></div>
</blockquote></div><br></div></div>