[llvm] r264902 - [PGO] PGOFuncName in LTO optimizations
Rong Xu via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 30 13:34:46 PDT 2016
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.
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160330/d53151b8/attachment.html>
More information about the llvm-commits
mailing list