llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-llvm-transforms Author: Owen Rodley (orodley) <details> <summary>Changes</summary> Simplify symtab GUID lookups by not emitting, reading, or writing legacy !PGOFuncName and !PGOName metadata in Clang and LLVM IR. Since we now have stable GUIDs, we no longer need to maintain the bespoke PGOName metadata to achieve the same purpose. This still includes a fallback to manual GUID calculation, as otherwise we break lots of tests. This will be removed and tests fixed separately. --- Patch is 25.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/214134.diff 11 Files Affected: - (modified) clang/lib/CodeGen/CodeGenPGO.cpp (-2) - (modified) llvm/include/llvm/ProfileData/InstrProf.h (+9-28) - (modified) llvm/lib/ProfileData/InstrProf.cpp (+23-94) - (modified) llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp (+1-1) - (modified) llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (+1-14) - (modified) llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp (+1-1) - (modified) llvm/test/Instrumentation/cgprofile.ll (+6-7) - (modified) llvm/test/Transforms/PGOProfile/icp_invoke.ll (+4-4) - (modified) llvm/test/Transforms/PGOProfile/icp_invoke_nouse.ll (+2-2) - (modified) llvm/test/Transforms/PGOProfile/thinlto_indirect_call_promotion.ll (+3-5) - (modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+9-9) ``````````diff diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp index 59faa3aef2460..1b16be8106f56 100644 --- a/clang/lib/CodeGen/CodeGenPGO.cpp +++ b/clang/lib/CodeGen/CodeGenPGO.cpp @@ -50,8 +50,6 @@ void CodeGenPGO::setFuncName(StringRef Name, void CodeGenPGO::setFuncName(llvm::Function *Fn) { setFuncName(Fn->getName(), Fn->getLinkage()); - // Create PGOFuncName meta data. - llvm::createPGOFuncNameMetadata(*Fn, FuncName); } /// The version of the PGO hash algorithm. diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h index cd07a012297ef..f2d8b35116fd3 100644 --- a/llvm/include/llvm/ProfileData/InstrProf.h +++ b/llvm/include/llvm/ProfileData/InstrProf.h @@ -228,7 +228,7 @@ inline StringRef getInstrProfNameSeparator() { return "\01"; } /// instrumentation LLVM_ABI bool isGPUProfTarget(const Module &M); -/// Please use getIRPGOFuncName for LLVM IR instrumentation. This function is +/// Please use getIRPGOObjectName for LLVM IR instrumentation. This function is /// for front-end (Clang, etc) instrumentation. /// Return the modified name for function \c F suitable to be /// used the key for profile lookup. Variable \c InLTO indicates if this @@ -245,33 +245,34 @@ LLVM_ABI std::string getPGOFuncName(StringRef RawFuncName, GlobalValue::LinkageTypes Linkage, StringRef FileName, uint64_t Version = INSTR_PROF_INDEX_VERSION); -/// \return the modified name for function \c F suitable to be +/// \return the modified name for global object \c GO suitable to be /// used as the key for IRPGO profile lookup. \c InLTO indicates if this is /// called from LTO optimization passes. -LLVM_ABI std::string getIRPGOFuncName(const Function &F, bool InLTO = false); +LLVM_ABI std::string getIRPGOObjectName(const GlobalObject &GO, + bool InLTO = false); /// \return the filename and the function name parsed from the output of -/// \c getIRPGOFuncName() +/// \c getIRPGOObjectName() LLVM_ABI std::pair<StringRef, StringRef> getParsedIRPGOName(StringRef IRPGOName); /// Return the name of the global variable used to store a function /// name in PGO instrumentation. \c FuncName is the IRPGO function name -/// (returned by \c getIRPGOFuncName) for LLVM IR instrumentation and PGO +/// (returned by \c getIRPGOObjectName) for LLVM IR instrumentation and PGO /// function name (returned by \c getPGOFuncName) for front-end instrumentation. LLVM_ABI std::string getPGOFuncNameVarName(StringRef FuncName, GlobalValue::LinkageTypes Linkage); /// Create and return the global variable for function name used in PGO /// instrumentation. \c FuncName is the IRPGO function name (returned by -/// \c getIRPGOFuncName) for LLVM IR instrumentation and PGO function name +/// \c getIRPGOObjectName) for LLVM IR instrumentation and PGO function name /// (returned by \c getPGOFuncName) for front-end instrumentation. LLVM_ABI GlobalVariable *createPGOFuncNameVar(Function &F, StringRef PGOFuncName); /// Create and return the global variable for function name used in PGO /// instrumentation. \c FuncName is the IRPGO function name (returned by -/// \c getIRPGOFuncName) for LLVM IR instrumentation and PGO function name +/// \c getIRPGOObjectName) for LLVM IR instrumentation and PGO function name /// (returned by \c getPGOFuncName) for front-end instrumentation. LLVM_ABI GlobalVariable *createPGOFuncNameVar(Module &M, GlobalValue::LinkageTypes Linkage, @@ -350,26 +351,6 @@ getValueProfDataFromInst(const Instruction &Inst, InstrProfValueKind ValueKind, uint32_t MaxNumValueData, uint64_t &TotalC, bool GetNoICPValue = false); -inline StringRef getPGOFuncNameMetadataName() { return "PGOFuncName"; } - -inline StringRef getPGONameMetadataName() { return "PGOName"; } - -/// Return the PGOFuncName meta data associated with a function. -LLVM_ABI MDNode *getPGOFuncNameMetadata(const Function &F); - -LLVM_ABI std::string getPGOName(const GlobalVariable &V, bool InLTO = false); - -/// Create the PGOFuncName meta data if PGOFuncName is different from -/// function's raw name. This should only apply to internal linkage functions -/// declared by users only. -/// TODO: Update all callers to 'createPGONameMetadata' and deprecate this -/// function. -LLVM_ABI void createPGOFuncNameMetadata(Function &F, StringRef PGOFuncName); - -/// Create the PGOName metadata if a global object's PGO name is different from -/// its mangled name. This should apply to local-linkage global objects only. -LLVM_ABI void createPGONameMetadata(GlobalObject &GO, StringRef PGOName); - /// Check if we can use Comdat for profile variables. This will eliminate /// the duplicated profile variables for Comdat functions. LLVM_ABI bool needsComdatForCounter(const GlobalObject &GV, const Module &M); @@ -513,7 +494,7 @@ uint64_t ComputeHash(StringRef K); /// A symbol table used for function [IR]PGO name look-up with keys /// (such as pointers, md5hash values) to the function. A function's /// [IR]PGO name or name's md5hash are used in retrieving the profile -/// data of the function. See \c getIRPGOFuncName() and \c getPGOFuncName +/// data of the function. See \c getIRPGOObjectName() and \c getPGOFuncName /// methods for details how [IR]PGO name is formed. class InstrProfSymtab { public: diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp index 6703d04c9493f..7dbc459c8a0e7 100644 --- a/llvm/lib/ProfileData/InstrProf.cpp +++ b/llvm/lib/ProfileData/InstrProf.cpp @@ -372,55 +372,24 @@ getIRPGONameForGlobalObject(const GlobalObject &GO, return GlobalValue::getGlobalIdentifier(GO.getName(), Linkage, FileName); } -static std::optional<std::string> lookupPGONameFromMetadata(MDNode *MD) { - if (MD != nullptr) { - StringRef S = cast<MDString>(MD->getOperand(0))->getString(); - return S.str(); - } - return {}; -} - // Returns the PGO object name. 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. -// Additionally, for ThinLTO mode, exported internal functions are promoted -// and renamed. We need to ensure that the original internal PGO name is -// used when computing the GUID that is compared against the profiled GUIDs. -// 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. -static std::string getIRPGOObjectName(const GlobalObject &GO, bool InLTO, - MDNode *PGONameMetadata) { +// when called in LTO optimization. In LTO mode (when InLTO is true), +// LTO's internalization privatizes many global linkage symbols, so we assume +// non-internal linkage without a source prefix. +std::string getIRPGOObjectName(const GlobalObject &GO, bool InLTO) { if (!InLTO) { auto FileName = getStrippedSourceFileName(GO); return getIRPGONameForGlobalObject(GO, GO.getLinkage(), FileName); } - // In LTO mode (when InLTO is true), first check if there is a meta data. - if (auto IRPGOFuncName = lookupPGONameFromMetadata(PGONameMetadata)) - return *IRPGOFuncName; - - // 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 getIRPGONameForGlobalObject(GO, GlobalValue::ExternalLinkage, ""); } -// Returns the IRPGO function name and does special handling when called -// in LTO optimization. See the comments of `getIRPGOObjectName` for details. -std::string getIRPGOFuncName(const Function &F, bool InLTO) { - return getIRPGOObjectName(F, InLTO, getPGOFuncNameMetadata(F)); -} - -// Please use getIRPGOFuncName for LLVM IR instrumentation. This function is +// Please use getIRPGOObjectName for LLVM IR instrumentation. This function is // for front-end (Clang, etc) instrumentation. // The implementation is kept for profile matching from older profiles. -// This is similar to `getIRPGOFuncName` except that this function calls -// 'getPGOFuncName' to get a name and `getIRPGOFuncName` calls +// This is similar to `getIRPGOObjectName` except that this function calls +// 'getPGOFuncName' to get a name and `getIRPGOObjectName` calls // 'getIRPGONameForGlobalObject'. See the difference between two callees in the // comments of `getIRPGONameForGlobalObject`. std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t Version) { @@ -429,23 +398,9 @@ std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t Version) { return getPGOFuncName(F.getName(), F.getLinkage(), FileName, Version); } - // In LTO mode (when InLTO is true), first check if there is a meta data. - if (auto PGOFuncName = lookupPGONameFromMetadata(getPGOFuncNameMetadata(F))) - return *PGOFuncName; - - // 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, ""); } -std::string getPGOName(const GlobalVariable &V, bool InLTO) { - // PGONameMetadata should be set by compiler at profile use time - // and read by symtab creation to look up symbols corresponding to - // a MD5 hash. - return getIRPGOObjectName(V, InLTO, V.getMetadata(getPGONameMetadataName())); -} - // See getIRPGOObjectName() for a discription of the format. std::pair<StringRef, StringRef> getParsedIRPGOName(StringRef IRPGOName) { auto [FileName, MangledName] = IRPGOName.split(GlobalIdentifierDelimiter); @@ -529,7 +484,7 @@ Error InstrProfSymtab::create(Module &M, bool InLTO, bool AddCanonical) { // Ignore in this case. if (!F.hasName()) continue; - auto IRPGOFuncName = getIRPGOFuncName(F, InLTO); + auto IRPGOFuncName = getIRPGOObjectName(F, InLTO); if (Error E = addFuncWithName(F, IRPGOFuncName, AddCanonical)) return E; // Also use getPGOFuncName() so that we can find records from older profiles @@ -542,7 +497,7 @@ Error InstrProfSymtab::create(Module &M, bool InLTO, bool AddCanonical) { for (GlobalVariable &G : M.globals()) { if (!G.hasName() || !G.hasMetadata(LLVMContext::MD_type)) continue; - if (Error E = addVTableWithName(G, getPGOName(G, InLTO))) + if (Error E = addVTableWithName(G, getIRPGOObjectName(G, InLTO))) return E; } @@ -558,8 +513,9 @@ Error InstrProfSymtab::addVTableWithName(GlobalVariable &VTable, return E; bool Inserted = true; - std::tie(std::ignore, Inserted) = MD5VTableMap.try_emplace( - GlobalValue::getGUIDAssumingExternalLinkage(Name), &VTable); + uint64_t GUID = VTable.getGUIDIfAssigned().value_or( + GlobalValue::getGUIDAssumingExternalLinkage(Name)); + std::tie(std::ignore, Inserted) = MD5VTableMap.try_emplace(GUID, &VTable); if (!Inserted) LLVM_DEBUG(dbgs() << "GUID conflict within one module"); return Error::success(); @@ -664,21 +620,22 @@ StringRef InstrProfSymtab::getCanonicalName(StringRef PGOName) { Error InstrProfSymtab::addFuncWithName(Function &F, StringRef PGOFuncName, bool AddCanonical) { - auto NameToGUIDMap = [&](StringRef Name) -> Error { - if (Error E = addFuncName(Name)) - return E; - MD5FuncMap.emplace_back(Function::getGUIDAssumingExternalLinkage(Name), &F); - return Error::success(); - }; - if (Error E = NameToGUIDMap(PGOFuncName)) + if (Error E = addFuncName(PGOFuncName)) return E; + uint64_t GUID = F.getGUIDIfAssigned().value_or( + Function::getGUIDAssumingExternalLinkage(PGOFuncName)); + MD5FuncMap.emplace_back(GUID, &F); if (!AddCanonical) return Error::success(); StringRef CanonicalFuncName = getCanonicalName(PGOFuncName); - if (!CanonicalFuncName.empty() && CanonicalFuncName != PGOFuncName) - return NameToGUIDMap(CanonicalFuncName); + if (!CanonicalFuncName.empty() && CanonicalFuncName != PGOFuncName) { + if (Error E = addFuncName(CanonicalFuncName)) + return E; + MD5FuncMap.emplace_back( + Function::getGUIDAssumingExternalLinkage(CanonicalFuncName), &F); + } return Error::success(); } @@ -769,7 +726,7 @@ Error collectVTableStrings(ArrayRef<GlobalVariable *> VTables, std::string &Result, bool DoCompression) { std::vector<std::string> VTableNameStrs; for (auto *VTable : VTables) - VTableNameStrs.push_back(getPGOName(*VTable)); + VTableNameStrs.push_back(getIRPGOObjectName(*VTable)); return collectGlobalObjectNameStrings( VTableNameStrs, compression::zlib::isAvailable() && DoCompression, Result); @@ -1543,34 +1500,6 @@ getValueProfDataFromInst(const Instruction &Inst, InstrProfValueKind ValueKind, return ValueData; } -MDNode *getPGOFuncNameMetadata(const Function &F) { - return F.getMetadata(getPGOFuncNameMetadataName()); -} - -static void createPGONameMetadata(GlobalObject &GO, StringRef MetadataName, - StringRef PGOName) { - // Only for internal linkage functions or global variables. The name is not - // the same as PGO name for these global objects. - if (GO.getName() == PGOName) - return; - - // Don't create duplicated metadata. - if (GO.getMetadata(MetadataName)) - return; - - LLVMContext &C = GO.getContext(); - MDNode *N = MDNode::get(C, MDString::get(C, PGOName)); - GO.setMetadata(MetadataName, N); -} - -void createPGOFuncNameMetadata(Function &F, StringRef PGOFuncName) { - return createPGONameMetadata(F, getPGOFuncNameMetadataName(), PGOFuncName); -} - -void createPGONameMetadata(GlobalObject &GO, StringRef PGOName) { - return createPGONameMetadata(GO, getPGONameMetadataName(), PGOName); -} - bool needsComdatForCounter(const GlobalObject &GO, const Module &M) { if (GO.hasComdat()) return true; diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp index fa46d10277ab4..5a3ead71b6a16 100644 --- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -1768,7 +1768,7 @@ void InstrLowerer::getOrCreateVTableProfData(GlobalVariable *GV) { // Used by INSTR_PROF_VTABLE_DATA MACRO Constant *VTableAddr = getVTableAddrForProfData(GV); - const std::string PGOVTableName = getPGOName(*GV); + const std::string PGOVTableName = getIRPGOObjectName(*GV); // Record the length of the vtable. This is needed since vtable pointers // loaded from C++ objects might be from the middle of a vtable definition. uint32_t VTableSizeVal = GV->getGlobalSize(M.getDataLayout()); diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp index 92cd9b33f2ef1..a7ea27de482b9 100644 --- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp +++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp @@ -675,7 +675,7 @@ template <class Edge, class BBInfo> class FuncPGOInstrumentation { NumOfCSPGOBB += MST.bbInfoSize(); } - FuncName = getIRPGOFuncName(F); + FuncName = getIRPGOObjectName(F); DeprecatedFuncName = getPGOFuncName(F); computeCFGHash(); if (!ComdatMembers.empty()) @@ -1895,9 +1895,6 @@ void PGOUseFunc::annotateValueSites() { if (DisableValueProfiling) return; - // Create the PGOFuncName meta data. - createPGOFuncNameMetadata(F, FuncInfo.FuncName); - for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) annotateValueSites(Kind); } @@ -2250,16 +2247,6 @@ static bool annotateAllFunctions( return false; } - if (EnableVTableProfileUse) { - for (GlobalVariable &G : M.globals()) { - if (!G.hasName() || !G.hasMetadata(LLVMContext::MD_type)) - continue; - - // Create the PGOFuncName meta data. - createPGONameMetadata(G, getPGOName(G, false /* InLTO*/)); - } - } - // Add the profile summary (read from the header of the indexed summary) here // so that we can use it below when reading counters (which checks if the // function should be marked with a cold or inlinehint attribute). diff --git a/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp b/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp index d46fea805df01..79bbcd3e87fb7 100644 --- a/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp +++ b/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp @@ -220,7 +220,7 @@ PreservedAnalyses JumpTableToSwitchPass::run(Function &F, return *MaybeGUID; return Function::getGUIDAssumingExternalLinkage( - getIRPGOFuncName(Fct, InLTO)); + getIRPGOObjectName(Fct, InLTO)); }; for (BasicBlock &BB : make_early_inc_range(F)) { diff --git a/llvm/test/Instrumentation/cgprofile.ll b/llvm/test/Instrumentation/cgprofile.ll index 72d10f6754c48..bf767a2301f32 100644 --- a/llvm/test/Instrumentation/cgprofile.ll +++ b/llvm/test/Instrumentation/cgprofile.ll @@ -1,5 +1,5 @@ -; RUN: opt < %s -passes='cg-profile<in-lto-post-link>' -S | FileCheck %s --check-prefixes=CHECK,LTO -; RUN: opt < %s -passes='cg-profile' -S | FileCheck %s --check-prefixes=CHECK,NOLTO --implicit-check-not="!{ptr @freq, ptr @func3.llvm.12345" +; RUN: opt < %s -passes='cg-profile<in-lto-post-link>' -S | FileCheck %s --check-prefixes=CHECK +; RUN: opt < %s -passes='cg-profile' -S | FileCheck %s --check-prefixes=CHECK declare void @b() @@ -8,7 +8,7 @@ define void @a() !prof !1 { ret void } -define void @func3.llvm.12345() !PGOFuncName !4 { +define void @func3.llvm.12345() !guid !4 { ret void } @@ -35,16 +35,15 @@ B: !1 = !{!"function_entry_count", i64 32} !2 = !{!"branch_weights", i32 5, i32 10} !3 = !{!"VP", i32 0, i64 1600, i64 7651369219802541373, i64 1030, i64 -4377547752858689819, i64 410, i64 5415368997850289431, i64 150, i64 -2545542355363006406, i64 10, i64 3667884930908592509, i64 1, i64 15435711456043681792, i64 0} -!4 = !{!"cgprofile.ll;func3"} +!4 = !{i64 5415368997850289431} ; CHECK: !llvm.module.flags = !{![[cgprof:[0-9]+]]} ; CHECK: ![[cgprof]] = !{i32 5, !"CG Profile", ![[prof:[0-9]+]]} -; LTO: ![[prof]] = distinct !{![[e0:[0-9]+]], ![[e1:[0-9]+]], ![[e2:[0-9]+]], ![[e3:[0-9]+]], ![[e4:[0-9]+]], ![[e5:[0-9]+]], ![[e6:[0-9]+]]} -; NOLTO: ![[prof]] = distinct !{![[e0:[0-9]+]], ![[e1:[0-9]+]], ![[e2:[0-9]+]], ![[e4:[0-9]+]], ![[e5:[0-9]+]], ![[e6:[0-9]+]]} +; CHECK: ![[prof]] = distinct !{![[e0:[0-9]+]], ![[e1:[0-9]+]], ![[e2:[0-9]+]], ![[e3:[0-9]+]], ![[e4:[0-9]+]], ![[e5:[0-9]+]], ![[e6:[0-9]+]]} ; CHECK: ![[e0]] = !{ptr @a, ptr @b, i64 32} ; CHECK: ![[e1]] = !{ptr @freq, ptr @func4, i64 1030} ; CHECK: ![[e2]] = !{ptr @freq, ptr @func2, i64 410} -; LTO: ![[e3]] = !{ptr @freq, ptr @func3.llvm.12345, i64 150} +; CHECK: ![[e3]] = !{ptr @freq, ptr @func3.llvm.12345, i64 150} ; CHECK: ![[e4]] = !{ptr @freq, ptr @func1, i64 10} ; CHECK: ![[e5]] = !{ptr @freq, ptr @a, i64 11} ; CHECK: ![[e6]] = !{ptr @freq, ptr @b, i64 21} diff --git a/llvm/test/Transforms/PGOProfile/icp_invoke.ll b/llvm/test/Transforms/PGOProfile/icp_invoke.ll index 04fc012c8458a..b7e0a608a29a6 100644 --- a/llvm/test/Transforms/PGOProfile/icp_invoke.ll +++ b/llvm/test/Transforms/PGOProfile/icp_invoke.ll @@ -6,12 +6,12 @@ target triple = "x86_64-unknown-linux-gnu" @foo2 = global ptr null, align 8 @_ZTIi = external constant ptr -define internal void @_ZL4bar1v() !PGOFuncName !0 { +define internal void @_ZL4bar1v() !guid !0 { entry: ret void } -define internal i32 @_ZL4bar2v() !PGOFuncName !1 { +define internal i32 @_ZL4bar2v() !guid !1 { entry: ret i32 100 } @@ -96,8 +96,8 @@ declare ptr @__cxa_begin_catch(ptr) declare void @__cxa_end_catch() -!0 = !{!"invoke.ll:_ZL4bar1v"} -!1 = !{!"invoke.ll:_ZL4bar2v"} +!0 =... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/214134 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
