<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 30, 2016 at 1:21 PM, Vedant Kumar <span dir="ltr"><<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Some inline comments and nits --<br>
<div><div><br>
> On Mar 30, 2016, at 1:01 PM, Justin Bogner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Rong Xu via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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>
<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.<br>
<br>
</div></div>Right, I'm worried about what changes (if any) are required to frontends<br>
which rely on getPGOFuncName(). @xur, you mentioned some follow-up work you<br>
were planning in clang. Could you elaborate on what's needed there?<br>
<div><div><br></div></div></blockquote><div>Note that this won't change anything with the edge profile.</div><div>It only changes the passes that called in LTO optimization passes -- not current pass using getPGOFuncName() in LTO yet. (My indirect call promotion pass will -- it's under review).</div><div><br></div><div>Clang's value profiling annotation will do the same thing -- create a PGOFuncName meta data.</div><div>I'll send a patch shortly.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
<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>
><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>
<br>
</div></div>nit: This would be more compact as `if (MDNode *MD = F.getMetadata(...)) {`.<br>
<span><br></span></blockquote><div>OK. This part will be refactored with the clang change. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
<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>
</span>nit: Please clang-format this last line.<br></blockquote><div>OK. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
<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>
</span>nit: How about `MD5FuncMap.emplace_back(Function::getGUID(...), &F)`?<br></blockquote><div>OK. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><font color="#888888"><br>
<br>
vedant<br>
</font></span><div><div><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" 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>
> _______________________________________________<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>
<br>
</div></div></blockquote></div><br></div></div>