[llvm] r264902 - [PGO] PGOFuncName in LTO optimizations
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 30 13:40:41 PDT 2016
On Wed, Mar 30, 2016 at 1:34 PM, Rong Xu via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
>
> On Wed, Mar 30, 2016 at 1:21 PM, Vedant Kumar <vsk at apple.com> wrote:
>
>> Some inline comments and nits --
>>
>> > On Mar 30, 2016, at 1:01 PM, Justin Bogner via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> >
>> > Rong Xu via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> >> Author: xur
>> >> Date: Wed Mar 30 13:37:52 2016
>> >> New Revision: 264902
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=264902&view=rev
>> >> Log:
>> >> [PGO] PGOFuncName in LTO optimizations
>> >>
>> >> PGOFuncNames are used as the key to retrieve the Function definition
>> from the
>> >> MD5 stored in the profile. For internal linkage function, we prefix
>> the source
>> >> file name to the PGOFuncNames. LTO's internalization privatizes many
>> >> global linkage
>> >> symbols. This happens after value profile annotation, but those
>> internal
>> >> linkage functions should not have a source prefix. To differentiate
>> compiler
>> >> generated internal symbols from original ones, PGOFuncName meta data
>> are
>> >> created and attached to the original internal symbols in the value
>> profile
>> >> annotation step. If a symbol does not have the meta data, its original
>> linkage
>> >> must be non-internal.
>> >>
>> >> Also add a new map that maps PGOFuncName's MD5 value to the function
>> definition.
>> >>
>> >> Differential Revision: http://reviews.llvm.org/D17895
>> >
>> > This review didn't show up on the LLVM list at all. Please make sure
>> > that code is reviewed on list in the future.
>>
>> > This might be okay for now, but the approach seems questionable. It's
>> > pretty awkward to have to teach profiling about LTO, and I feel like the
>> > problems with how we munge internal names come up in other contexts
>> > too.
>>
>> Right, I'm worried about what changes (if any) are required to frontends
>> which rely on getPGOFuncName(). @xur, you mentioned some follow-up work
>> you
>> were planning in clang. Could you elaborate on what's needed there?
>>
>> Note that this won't change anything with the edge profile.
> 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).
>
> Clang's value profiling annotation will do the same thing -- create a
> PGOFuncName meta data.
> I'll send a patch shortly.
>
I think what you mean is that Clang based value profiling annotation has
the same issue in LTO mode, which can be fixed the same way.
David
>
>
>> > Longer term, I think that the right thing to do in terms of dealing with
>> > internal functions is to avoid munging their names at all. To do this,
>> > we'd need to add something to the function hash or as a new field to
>> > avoid collisions between unrelated local names, but such an approach
>> > would clean up the whole class of issues with how we rename these
>> > symbols.
>> >
>> >> Modified:
>> >> llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> >> llvm/trunk/lib/ProfileData/InstrProf.cpp
>> >> llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
>> >>
>> >> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> >> URL:
>> >>
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=264902&r1=264901&r2=264902&view=diff
>> >>
>> ==============================================================================
>> >> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> >> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Mar 30
>> 13:37:52 2016
>> >> @@ -155,8 +155,9 @@ inline StringRef getInstrProfFileOverrid
>> >> inline StringRef getInstrProfNameSeparator() { return "\01"; }
>> >>
>> >> /// Return the modified name for function \c F suitable to be
>> >> -/// used the key for profile lookup.
>> >> -std::string getPGOFuncName(const Function &F,
>> >> +/// used the key for profile lookup. Variable \c InLTO indicates if
>> this
>> >> +/// is called in LTO optimization passes.
>> >> +std::string getPGOFuncName(const Function &F, bool InLTO = false,
>> >> uint64_t Version =
>> INSTR_PROF_INDEX_VERSION);
>> >>
>> >> /// Return the modified name for a function suitable to be
>> >> @@ -302,13 +303,17 @@ private:
>> >> StringSet<> NameTab;
>> >> // A map from MD5 keys to function name strings.
>> >> std::vector<std::pair<uint64_t, StringRef>> MD5NameMap;
>> >> + // A map from MD5 keys to function define. We only populate this map
>> >> + // when build the Symtab from a Module.
>> >> + std::vector<std::pair<uint64_t, Function *>> MD5FuncMap;
>> >> // A map from function runtime address to function name MD5 hash.
>> >> // This map is only populated and used by raw instr profile reader.
>> >> AddrHashMap AddrToMD5Map;
>> >>
>> >> public:
>> >> InstrProfSymtab()
>> >> - : Data(), Address(0), NameTab(), MD5NameMap(), AddrToMD5Map() {}
>> >> + : Data(), Address(0), NameTab(), MD5NameMap(), MD5FuncMap(),
>> >> + AddrToMD5Map() {}
>> >>
>> >> /// Create InstrProfSymtab from an object file section which
>> >> /// contains function PGO names. When section may contain raw
>> >> @@ -326,8 +331,9 @@ public:
>> >> inline std::error_code create(StringRef NameStrings);
>> >> /// A wrapper interface to populate the PGO symtab with functions
>> >> /// decls from module \c M. This interface is used by transformation
>> >> - /// passes such as indirect function call promotion.
>> >> - void create(const Module &M);
>> >> + /// passes such as indirect function call promotion. Variable \c
>> InLTO
>> >> + /// indicates if this is called from LTO optimization passes.
>> >> + void create(Module &M, bool InLTO = false);
>> >> /// Create InstrProfSymtab from a set of names iteratable from
>> >> /// \p IterRange. This interface is used by IndexedProfReader.
>> >> template <typename NameIterRange> void create(const NameIterRange
>> &IterRange);
>> >> @@ -357,6 +363,8 @@ public:
>> >> /// Return function's PGO name from the name's md5 hash value.
>> >> /// If not found, return an empty string.
>> >> inline StringRef getFuncName(uint64_t FuncMD5Hash);
>> >> + /// Return function from the name's md5 hash. Return nullptr if not
>> found.
>> >> + inline Function *getFunction(uint64_t FuncMD5Hash);
>> >> /// Return the function's original assembly name by stripping off
>> >> /// the prefix attached (to symbols with priviate linkage). For
>> >> /// global functions, it returns the same string as getFuncName.
>> >> @@ -387,6 +395,7 @@ void InstrProfSymtab::create(const NameI
>> >>
>> >> void InstrProfSymtab::finalizeSymtab() {
>> >> std::sort(MD5NameMap.begin(), MD5NameMap.end(), less_first());
>> >> + std::sort(MD5FuncMap.begin(), MD5FuncMap.end(), less_first());
>> >> std::sort(AddrToMD5Map.begin(), AddrToMD5Map.end(), less_first());
>> >> AddrToMD5Map.erase(std::unique(AddrToMD5Map.begin(),
>> AddrToMD5Map.end()),
>> >> AddrToMD5Map.end());
>> >> @@ -402,6 +411,16 @@ StringRef InstrProfSymtab::getFuncName(u
>> >> return StringRef();
>> >> }
>> >>
>> >> +Function* InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) {
>> >> + auto Result =
>> >> + std::lower_bound(MD5FuncMap.begin(), MD5FuncMap.end(),
>> FuncMD5Hash,
>> >> + [](const std::pair<uint64_t, Function*> &LHS,
>> >> + uint64_t RHS) { return LHS.first < RHS; });
>> >> + if (Result != MD5FuncMap.end() && Result->first == FuncMD5Hash)
>> >> + return Result->second;
>> >> + return nullptr;
>> >> +}
>> >> +
>> >> // See also getPGOFuncName implementation. These two need to be
>> >> // matched.
>> >> StringRef InstrProfSymtab::getOrigFuncName(uint64_t FuncMD5Hash) {
>> >>
>> >> Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
>> >> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=264902&r1=264901&r2=264902&view=diff
>> >>
>> ==============================================================================
>> >> --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
>> >> +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Mar 30 13:37:52 2016
>> >> @@ -83,9 +83,32 @@ std::string getPGOFuncName(StringRef Raw
>> >> return GlobalValue::getGlobalIdentifier(RawFuncName, Linkage,
>> FileName);
>> >> }
>> >>
>> >> -std::string getPGOFuncName(const Function &F, uint64_t Version) {
>> >> - return getPGOFuncName(F.getName(), F.getLinkage(),
>> F.getParent()->getName(),
>> >> - Version);
>> >> +// Return the PGOFuncName. This function has some special handling
>> when called
>> >> +// in LTO optimization. The following only applies when calling in
>> LTO passes
>> >> +// (when \c InLTO is true): LTO's internalization privatizes many
>> global linkage
>> >> +// symbols. This happens after value profile annotation, but those
>> internal
>> >> +// linkage functions should not have a source prefix.
>> >> +// To differentiate compiler generated internal symbols from original
>> ones,
>> >> +// PGOFuncName meta data are created and attached to the original
>> internal
>> >> +// symbols in the value profile annotation step
>> >> +// (PGOUseFunc::annotateIndirectCallSites). If a symbol does not have
>> the meta
>> >> +// data, its original linkage must be non-internal.
>> >> +std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t
>> Version) {
>> >> + if (!InLTO)
>> >> + return getPGOFuncName(F.getName(), F.getLinkage(),
>> F.getParent()->getName(),
>> >> + Version);
>> >> +
>> >> + // InLTO mode. First check if these is a meta data.
>> >> + MDNode *MD = F.getMetadata("PGOFuncName");
>> >> + if (MD != nullptr) {
>>
>> nit: This would be more compact as `if (MDNode *MD = F.getMetadata(...))
>> {`.
>>
>> OK. This part will be refactored with the clang change.
>
>>
>> >> + StringRef S = cast<MDString>(MD->getOperand(0))->getString();
>> >> + return S.str();
>> >> + }
>> >> +
>> >> + // If there is no meta data, the function must be a global before
>> the value
>> >> + // profile annotation pass. Its current linkage may be internal if
>> it is
>> >> + // internalized in LTO mode.
>> >> + return getPGOFuncName (F.getName(), GlobalValue::ExternalLinkage,
>> "");
>>
>> nit: Please clang-format this last line.
>>
> OK.
>
>>
>>
>> >> }
>> >>
>> >> StringRef getFuncNameWithoutPrefix(StringRef PGOFuncName, StringRef
>> FileName) {
>> >> @@ -149,9 +172,16 @@ GlobalVariable *createPGOFuncNameVar(Fun
>> >> return createPGOFuncNameVar(*F.getParent(), F.getLinkage(),
>> PGOFuncName);
>> >> }
>> >>
>> >> -void InstrProfSymtab::create(const Module &M) {
>> >> - for (const Function &F : M)
>> >> - addFuncName(getPGOFuncName(F));
>> >> +void InstrProfSymtab::create(Module &M, bool InLTO) {
>> >> + for (Function &F : M) {
>> >> + // Function may not have a name: like using asm("") to overwrite
>> the name.
>> >> + // Ignore in this case.
>> >> + if (!F.hasName())
>> >> + continue;
>> >> + const std::string &PGOFuncName = getPGOFuncName(F, InLTO);
>> >> + addFuncName(PGOFuncName);
>> >> +
>> MD5FuncMap.push_back(std::make_pair(Function::getGUID(PGOFuncName), &F));
>>
>> nit: How about `MD5FuncMap.emplace_back(Function::getGUID(...), &F)`?
>>
> OK.
>
>>
>>
>> vedant
>>
>> >> + }
>> >>
>> >> finalizeSymtab();
>> >> }
>> >>
>> >> Modified:
>> llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
>> >> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp?rev=264902&r1=264901&r2=264902&view=diff
>> >>
>> ==============================================================================
>> >> --- llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
>> (original)
>> >> +++ llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
>> Wed Mar 30 13:37:52 2016
>> >> @@ -750,6 +750,15 @@ void PGOUseFunc::annotateIndirectCallSit
>> >> if (DisableValueProfiling)
>> >> return;
>> >>
>> >> + // Write out the PGOFuncName if this is different from it's raw
>> name.
>> >> + // This should only apply to internal linkage functions only.
>> >> + const std::string &FuncName = getPGOFuncName(F);
>> >> + if (FuncName != F.getName()) {
>> >> + LLVMContext &C = F.getContext();
>> >> + MDNode *N = MDNode::get(C, MDString::get(C, FuncName.c_str()));
>> >> + F.setMetadata("PGOFuncName", N);
>> >> + }
>> >> +
>> >> unsigned IndirectCallSiteIndex = 0;
>> >> PGOIndirectCallSiteVisitor ICV;
>> >> ICV.visit(F);
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160330/7ab976c8/attachment.html>
More information about the llvm-commits
mailing list