<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 30, 2016 at 1:01 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Rong Xu via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
> Author: xur<br>
> Date: Wed Mar 30 13:37:52 2016<br>
> New Revision: 264902<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=264902&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=264902&view=rev</a><br>
> Log:<br>
> [PGO] PGOFuncName in LTO optimizations<br>
><br>
> PGOFuncNames are used as the key to retrieve the Function definition from the<br>
> MD5 stored in the profile. For internal linkage function, we prefix the source<br>
> file name to the PGOFuncNames. LTO's internalization privatizes many<br>
> global linkage<br>
> symbols. This happens after value profile annotation, but those internal<br>
> linkage functions should not have a source prefix. To differentiate compiler<br>
> generated internal symbols from original ones, PGOFuncName meta data are<br>
> created and attached to the original internal symbols in the value profile<br>
> annotation step. If a symbol does not have the meta data, its original linkage<br>
> must be non-internal.<br>
><br>
> Also add a new map that maps PGOFuncName's MD5 value to the function definition.<br>
><br>
> Differential Revision: <a href="http://reviews.llvm.org/D17895" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17895</a><br>
<br>
This review didn't show up on the LLVM list at all. Please make sure<br>
that code is reviewed on list in the future.<br></blockquote><div><br></div><div>Sorry. This is my fault. I thought I had add llvm-commits to the subscriber, but obviously I did not.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
This might be okay for now, but the approach seems questionable. It's<br>
pretty awkward to have to teach profiling about LTO, and I feel like the<br>
problems with how we munge internal names come up in other contexts<br>
too. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Longer term, I think that the right thing to do in terms of dealing with<br>
internal functions is to avoid munging their names at all. To do this,<br>
we'd need to add something to the function hash or as a new field to<br>
avoid collisions between unrelated local names, but such an approach<br>
would clean up the whole class of issues with how we rename these<br>
symbols.<br></blockquote><div><br></div><div>I agree there are better ways to handle the conflict internal names than current source prefixing method. </div><div>But I think it's gonna hard to avoid the avoid the interaction with LTO (I mean the internalization). Note that the profile-generation pass is pre LTO. The compiler has to have different handling of originally internal symbols and later generated internal symbols. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Modified:<br>
> llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
> llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
> llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=264902&r1=264901&r2=264902&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=264902&r1=264901&r2=264902&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)<br>
> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Mar 30 13:37:52 2016<br>
> @@ -155,8 +155,9 @@ inline StringRef getInstrProfFileOverrid<br>
> inline StringRef getInstrProfNameSeparator() { return "\01"; }<br>
><br>
> /// Return the modified name for function \c F suitable to be<br>
> -/// used the key for profile lookup.<br>
> -std::string getPGOFuncName(const Function &F,<br>
> +/// used the key for profile lookup. Variable \c InLTO indicates if this<br>
> +/// is called in LTO optimization passes.<br>
> +std::string getPGOFuncName(const Function &F, bool InLTO = false,<br>
> uint64_t Version = INSTR_PROF_INDEX_VERSION);<br>
><br>
> /// Return the modified name for a function suitable to be<br>
> @@ -302,13 +303,17 @@ private:<br>
> StringSet<> NameTab;<br>
> // A map from MD5 keys to function name strings.<br>
> std::vector<std::pair<uint64_t, StringRef>> MD5NameMap;<br>
> + // A map from MD5 keys to function define. We only populate this map<br>
> + // when build the Symtab from a Module.<br>
> + std::vector<std::pair<uint64_t, Function *>> MD5FuncMap;<br>
> // A map from function runtime address to function name MD5 hash.<br>
> // This map is only populated and used by raw instr profile reader.<br>
> AddrHashMap AddrToMD5Map;<br>
><br>
> public:<br>
> InstrProfSymtab()<br>
> - : Data(), Address(0), NameTab(), MD5NameMap(), AddrToMD5Map() {}<br>
> + : Data(), Address(0), NameTab(), MD5NameMap(), MD5FuncMap(),<br>
> + AddrToMD5Map() {}<br>
><br>
> /// Create InstrProfSymtab from an object file section which<br>
> /// contains function PGO names. When section may contain raw<br>
> @@ -326,8 +331,9 @@ public:<br>
> inline std::error_code create(StringRef NameStrings);<br>
> /// A wrapper interface to populate the PGO symtab with functions<br>
> /// decls from module \c M. This interface is used by transformation<br>
> - /// passes such as indirect function call promotion.<br>
> - void create(const Module &M);<br>
> + /// passes such as indirect function call promotion. Variable \c InLTO<br>
> + /// indicates if this is called from LTO optimization passes.<br>
> + void create(Module &M, bool InLTO = false);<br>
> /// Create InstrProfSymtab from a set of names iteratable from<br>
> /// \p IterRange. This interface is used by IndexedProfReader.<br>
> template <typename NameIterRange> void create(const NameIterRange &IterRange);<br>
> @@ -357,6 +363,8 @@ public:<br>
> /// Return function's PGO name from the name's md5 hash value.<br>
> /// If not found, return an empty string.<br>
> inline StringRef getFuncName(uint64_t FuncMD5Hash);<br>
> + /// Return function from the name's md5 hash. Return nullptr if not found.<br>
> + inline Function *getFunction(uint64_t FuncMD5Hash);<br>
> /// Return the function's original assembly name by stripping off<br>
> /// the prefix attached (to symbols with priviate linkage). For<br>
> /// global functions, it returns the same string as getFuncName.<br>
> @@ -387,6 +395,7 @@ void InstrProfSymtab::create(const NameI<br>
><br>
> void InstrProfSymtab::finalizeSymtab() {<br>
> std::sort(MD5NameMap.begin(), MD5NameMap.end(), less_first());<br>
> + std::sort(MD5FuncMap.begin(), MD5FuncMap.end(), less_first());<br>
> std::sort(AddrToMD5Map.begin(), AddrToMD5Map.end(), less_first());<br>
> AddrToMD5Map.erase(std::unique(AddrToMD5Map.begin(), AddrToMD5Map.end()),<br>
> AddrToMD5Map.end());<br>
> @@ -402,6 +411,16 @@ StringRef InstrProfSymtab::getFuncName(u<br>
> return StringRef();<br>
> }<br>
><br>
> +Function* InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) {<br>
> + auto Result =<br>
> + std::lower_bound(MD5FuncMap.begin(), MD5FuncMap.end(), FuncMD5Hash,<br>
> + [](const std::pair<uint64_t, Function*> &LHS,<br>
> + uint64_t RHS) { return LHS.first < RHS; });<br>
> + if (Result != MD5FuncMap.end() && Result->first == FuncMD5Hash)<br>
> + return Result->second;<br>
> + return nullptr;<br>
> +}<br>
> +<br>
> // See also getPGOFuncName implementation. These two need to be<br>
> // matched.<br>
> StringRef InstrProfSymtab::getOrigFuncName(uint64_t FuncMD5Hash) {<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=264902&r1=264901&r2=264902&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=264902&r1=264901&r2=264902&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)<br>
> +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Mar 30 13:37:52 2016<br>
> @@ -83,9 +83,32 @@ std::string getPGOFuncName(StringRef Raw<br>
> return GlobalValue::getGlobalIdentifier(RawFuncName, Linkage, FileName);<br>
> }<br>
><br>
> -std::string getPGOFuncName(const Function &F, uint64_t Version) {<br>
> - return getPGOFuncName(F.getName(), F.getLinkage(), F.getParent()->getName(),<br>
> - Version);<br>
> +// Return the PGOFuncName. This function has some special handling when called<br>
> +// in LTO optimization. The following only applies when calling in LTO passes<br>
> +// (when \c InLTO is true): LTO's internalization privatizes many global linkage<br>
> +// symbols. This happens after value profile annotation, but those internal<br>
> +// linkage functions should not have a source prefix.<br>
> +// To differentiate compiler generated internal symbols from original ones,<br>
> +// PGOFuncName meta data are created and attached to the original internal<br>
> +// symbols in the value profile annotation step<br>
> +// (PGOUseFunc::annotateIndirectCallSites). If a symbol does not have the meta<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>
> + return getPGOFuncName(F.getName(), F.getLinkage(), F.getParent()->getName(),<br>
> + Version);<br>
> +<br>
> + // InLTO mode. First check if these is a meta data.<br>
> + MDNode *MD = F.getMetadata("PGOFuncName");<br>
> + if (MD != nullptr) {<br>
> + StringRef S = cast<MDString>(MD->getOperand(0))->getString();<br>
> + return S.str();<br>
> + }<br>
> +<br>
> + // If there is no meta data, the function must be a global before the value<br>
> + // profile annotation pass. Its current linkage may be internal if it is<br>
> + // internalized in LTO mode.<br>
> + return getPGOFuncName (F.getName(), GlobalValue::ExternalLinkage, "");<br>
> }<br>
><br>
> StringRef getFuncNameWithoutPrefix(StringRef PGOFuncName, StringRef FileName) {<br>
> @@ -149,9 +172,16 @@ GlobalVariable *createPGOFuncNameVar(Fun<br>
> return createPGOFuncNameVar(*F.getParent(), F.getLinkage(), PGOFuncName);<br>
> }<br>
><br>
> -void InstrProfSymtab::create(const Module &M) {<br>
> - for (const Function &F : M)<br>
> - addFuncName(getPGOFuncName(F));<br>
> +void InstrProfSymtab::create(Module &M, bool InLTO) {<br>
> + for (Function &F : M) {<br>
> + // Function may not have a name: like using asm("") to overwrite the name.<br>
> + // Ignore in this case.<br>
> + if (!F.hasName())<br>
> + continue;<br>
> + const std::string &PGOFuncName = getPGOFuncName(F, InLTO);<br>
> + addFuncName(PGOFuncName);<br>
> + MD5FuncMap.push_back(std::make_pair(Function::getGUID(PGOFuncName), &F));<br>
> + }<br>
><br>
> finalizeSymtab();<br>
> }<br>
><br>
> Modified: llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp?rev=264902&r1=264901&r2=264902&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp?rev=264902&r1=264901&r2=264902&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp Wed Mar 30 13:37:52 2016<br>
> @@ -750,6 +750,15 @@ void PGOUseFunc::annotateIndirectCallSit<br>
> if (DisableValueProfiling)<br>
> return;<br>
><br>
> + // Write out the PGOFuncName if this is different from it's raw name.<br>
> + // This should only apply to internal linkage functions only.<br>
> + const std::string &FuncName = getPGOFuncName(F);<br>
> + if (FuncName != F.getName()) {<br>
> + LLVMContext &C = F.getContext();<br>
> + MDNode *N = MDNode::get(C, MDString::get(C, FuncName.c_str()));<br>
> + F.setMetadata("PGOFuncName", N);<br>
> + }<br>
> +<br>
> unsigned IndirectCallSiteIndex = 0;<br>
> PGOIndirectCallSiteVisitor ICV;<br>
> ICV.visit(F);<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">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></div></div>