<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 22, 2019 at 10:24 PM Rong Xu <<a href="mailto:xur@google.com">xur@google.com</a>> wrote:<br></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 dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail_attr">On Tue, Jan 22, 2019 at 10:03 PM Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br></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 dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail_attr">On Tue, Jan 22, 2019 at 9:17 PM Rong Xu <<a href="mailto:xur@google.com" target="_blank">xur@google.com</a>> wrote:<br></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 dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail_attr">On Tue, Jan 22, 2019 at 5:51 PM Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br></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 dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail_attr">On Tue, Jan 22, 2019 at 5:03 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></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"><br><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail-m_8234649074384883388gmail_attr">On Tue, Jan 22, 2019 at 4:10 PM Rong Xu <<a href="mailto:xur@google.com" target="_blank">xur@google.com</a>> wrote:<br></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_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail-m_8234649074384883388m_8251850977454740452gmail_attr">On Tue, Jan 22, 2019 at 2:44 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></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"><br><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail-m_8234649074384883388m_8251850977454740452gmail-m_1737551164075983742gmail_attr">On Tue, Jan 22, 2019 at 2:39 PM Rong Xu <<a href="mailto:xur@google.com" target="_blank">xur@google.com</a>> wrote:<br></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_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail-m_8234649074384883388m_8251850977454740452gmail-m_1737551164075983742m_-5559548774396098271gmail_attr">On Tue, Jan 22, 2019 at 1:24 PM Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br></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 dir="ltr"><div>On Tue, Jan 22, 2019 at 1:06 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><div>></div><div>> Hey Rong, Teresa,</div><div>></div><div>> This seems like it might be problematic to me - Couldn't multiple modules have the same source file name (built with different preprocessor defines, etc) - at least I think that's the case for some projects at Google.</div><div>></div><div>> Does this identifier need to be unique? What are the ramifications if multiple modules had the same source file name & this situation?</div><div><br></div><div>Rong will know for sure, but I think the profiles will be merged and it is possible there will be a profile mismatch.</div><div><br></div><div>However, note that this is what already happens for PGO - this change retains the status quo since PGO is matched during the compile from source where the module name (F.getParent()->getName()) is the same as F.getParent()->getSourceFileName(). So any uniqueness issue already exists with PGO.</div><div><br></div><div>This change is to enable context sensitive PGO which must be done in the *LTO backends, where we have recorded the original module name in the SourceFileName in the bitcode (in the *LTO backends the module name is the .o bitcode file name).</div><div><br></div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>Thanks for Teresa explanation and the background. I just want to add one more thing: we use both filename and function's CFG checksum in the static function's PGOFuncName. If multiple functions have the same source name, but with different CFG checksums, they can coexist in the profile. Profile-use compilation will pick the exact match one. In theory, there still exist some collisions, but the chance are very small.</div></div></div></blockquote><div><br></div><div>So if this was the original source file name, it wouldn't be too hard to collide, even with the CFG checksum mixed in, I think. In Google, at least, it's not too uncommon (I've seen it a few times, at least) for a source file to be built into multiple libraries, either by mistake or intentionally (if it's intentional, then the builds probably use different parameters - different #defines and the like) - and a file-local static function in such a file might not depend on those #defines or other parameters, so it would be the same across files - while still being distinct functions.<br><br></div></div></div></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div>For Google applications, we use distributed thinlto build. The patch won't affect them as the IR files (with .o suffix) is passed the post LTO compilation. This patch will treat the .o as the source files.</div></div></div></blockquote></div></div></blockquote><div><br></div><div>This is not correct. The source file name (the module id when we compile from cc to bitcode .o) is encoded in the bitcode file:</div><div><a href="https://llvm.org/docs/LangRef.html#source-filename" target="_blank">https://llvm.org/docs/LangRef.html#source-filename</a><br></div><div><br></div><div>When the .o bitcode file is processed through the LTO backend, regardless of whether it is in process or distributed, that will be saved in the sourceFileName field on the module, which is what is being accessed here. So it will always be the actual source .cc file name - the same as the module id in the .cc->.o compile step, which is why this change is a no-op for existing PGO which is done during the compile step.</div><div><br></div></div></div></div></blockquote><div>We are talking different things: Teresa is talking the callers to this function for indirectly promotion or importing. In these callers, the parameter of InLTO is set. So we will use the metadata.</div><div>What I was referring to is in the context sensitive patch, where the caller to this function does not set InLTO flag. </div><div>The reason for this is many functions are internaliazed after PGOInstrumentaiton pass (where the metadata is set).</div><div>When we call from there, we will use the IR module name (.o) as the source name. If you check the profile for context sensitive profile, there are many entries like bar.o:foo()</div></div></div></blockquote><div><br></div><div>I'm confused, since with !InLTO it should go into the code block modified with this patch, which means that it should use F.getParent()->getSourceFileName() instead of F.getParent()->getName(). The SourceFileName should be the original source (.cc) name not the .o file name. The SourceFileName should be set on the Module when reading the bitcode in the backend, from the source file name recorded when we compiled the source to bitcode. For that reason, before this patch I would have expected to see names like "bar.o:foo" but with this patch I would expect that to instead be "bar.cc:foo". How are we getting "bar.o" in the SourceFileName field on the Module? Sorry if I am misunderstanding...</div><div><br></div></div></div></blockquote><div>I run the test again. Yes. You are right. In thinlto, regardless distributed mode or not, we should see bar.cc:foo.</div><div>I was under the impression F.getParent()->getSourceFileName() would return the IR file name (.o). But that was wrong.</div><div>Obviously, thinlto importer will set the source file name from the IR file.</div><div>It is good thing that distributed mode and ld-plugin have the same bahaviors. </div></div></div></blockquote><div><br></div><div>Ok great. Right, they should be consistent.</div><div><br></div><div>I'd suggest changing the name of the IsLTO flag as it is now somewhat confusing (since we will do CS PGO during the LTO step too). It's really specific to ICP done during the LTO step for PGO data matched earlier. Maybe change it to "InLTOICP"? </div><div><br></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_quote"><div><br></div><div>For fulllto, F.getParent()->getSourceFileName() return ld-temp.o. Not sure if this is a bug.</div></div></div></blockquote><div><br></div><div>That's because for full LTO there isn't a single source module - this is essentially a new module created from all of the regular LTO modules combined. Presumably this name should be fine as it will be consistent between gen/use compiles.</div><div><br></div><div>Teresa</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_quote"><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_quote"><div></div><div>Teresa</div><div><br></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_quote"><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 dir="ltr"><div class="gmail_quote"><div></div><div>Teresa</div><div><br></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_quote"><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_quote"><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_quote"><div>But if the filename being used is the name of the intermediate .o file rather than the original source file, well that filename has to be unique for the linker to be able to read both .o files (even if they came from the same source file) & so it should provide a good uniqueness.<br><br></div></div></div></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div>This only affects the compilation through linker plugin. Without this patch, if we call that function after LTO linking, all the static function will have a mismatch. As the profile-use and profile-gen have different module name (with a unique temporary string in the file name). This is much worse than the collision.</div></div></div></blockquote><div><br>Ah, OK - so what's the source name if the input to the linker is an IR file (with .o suffix) as would usually be the case (but with linker plugin rather than distributed thinlto) - is it still the .o file name passed to the linekr? or is it now the user source code file name? If it's still the .o file name, that'd still avoid a collision, I think?<br><br>- Dave<br> </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_quote"><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_quote"><div>- Dave</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_quote"><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 dir="ltr"><div></div><div>Teresa</div><div><br></div><div>></div><div>> - Dave</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail-m_8234649074384883388m_8251850977454740452gmail-m_1737551164075983742m_-5559548774396098271gmail-m_-6172247499348909344gmail_attr">On Tue, Jan 8, 2019 at 2:43 PM Rong Xu via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: xur<br>
Date: Tue Jan 8 14:39:47 2019<br>
New Revision: 350671<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=350671&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=350671&view=rev</a><br>
Log:<br>
[PGO] Use SourceFileName rather module name in PGOFuncName<br>
<br>
In LTO or Thin-lto mode (though linker plugin), the module<br>
names are of temp file names which are different for<br>
different compilations. Using SourceFileName avoids the issue.<br>
This should not change any functionality for current PGO as<br>
all the current callers of getPGOFuncName() is before LTO.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D56327" rel="noreferrer" target="_blank">https://reviews.llvm.org/D56327</a><br>
<br>
Modified:<br>
llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
<br>
Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=350671&r1=350670&r2=350671&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=350671&r1=350670&r2=350671&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)<br>
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Tue Jan 8 14:39:47 2019<br>
@@ -252,11 +252,12 @@ static StringRef stripDirPrefix(StringRe<br>
// data, its original linkage must be non-internal.<br>
std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t Version) {<br>
if (!InLTO) {<br>
- StringRef FileName = (StaticFuncFullModulePrefix<br>
- ? F.getParent()->getName()<br>
- : sys::path::filename(F.getParent()->getName()));<br>
- if (StaticFuncFullModulePrefix && StaticFuncStripDirNamePrefix != 0)<br>
- FileName = stripDirPrefix(FileName, StaticFuncStripDirNamePrefix);<br>
+ StringRef FileName(F.getParent()->getSourceFileName());<br>
+ uint32_t StripLevel = StaticFuncFullModulePrefix ? 0 : (uint32_t)-1;<br>
+ if (StripLevel < StaticFuncStripDirNamePrefix)<br>
+ StripLevel = StaticFuncStripDirNamePrefix;<br>
+ if (StripLevel)<br>
+ FileName = stripDirPrefix(FileName, StripLevel);<br>
return getPGOFuncName(F.getName(), F.getLinkage(), FileName, Version);<br>
}<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail-m_8234649074384883388m_8251850977454740452gmail-m_1737551164075983742m_-5559548774396098271gmail-m_-6172247499348909344gmail_signature"><div dir="ltr"><div><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:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"><br></td></tr></tbody></table></span></div></div></div>
</blockquote></div></div></blockquote></div></div>
</blockquote></div></div></blockquote></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail-m_-8921182573818788517gmail-m_-8507737427730794713gmail_signature"><div dir="ltr"><div><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:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"><br></td></tr></tbody></table></span></div></div></div></div></div>
</blockquote></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail-m_1001875005022640891gmail-m_-5925498358865907240gmail_signature"><div dir="ltr"><div><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:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"><br></td></tr></tbody></table></span></div></div></div></div>
</blockquote></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><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:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"><br></td></tr></tbody></table></span></div></div></div></div>