Compute GUIDs once and store in metadata#184065
Merged
Merged
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
mtrofin
reviewed
Mar 8, 2026
llvmbot
added
backend:X86
PGO
Member
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-backend-x86 Author: Owen Rodley (orodley) ChangesThis allows us to keep GUIDs consistent across compilation phases which may change the name or linkage type. See https://discourse.llvm.org/t/rfc-keep-globalvalue-guids-stable/84801 Patch is 117.67 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/184065.diff 87 Files Affected:
diff --git a/llvm/include/llvm/Analysis/CtxProfAnalysis.h b/llvm/include/llvm/Analysis/CtxProfAnalysis.h
index 5c9823b0f6cc1..90b1d3fc0a688 100644
--- a/llvm/include/llvm/Analysis/CtxProfAnalysis.h
+++ b/llvm/include/llvm/Analysis/CtxProfAnalysis.h
@@ -47,9 +47,6 @@ class PGOContextualProfile {
// we'll need when we maintain the profiles during IPO transformations.
std::map<GlobalValue::GUID, FunctionInfo> FuncInfo;
- /// Get the GUID of this Function if it's defined in this module.
- LLVM_ABI GlobalValue::GUID getDefinedFunctionGUID(const Function &F) const;
-
// This is meant to be constructed from CtxProfAnalysis, which will also set
// its state piecemeal.
PGOContextualProfile() = default;
@@ -68,9 +65,7 @@ class PGOContextualProfile {
LLVM_ABI bool isInSpecializedModule() const;
- bool isFunctionKnown(const Function &F) const {
- return getDefinedFunctionGUID(F) != 0;
- }
+ bool isFunctionKnown(const Function &F) const { return !F.isDeclaration(); }
StringRef getFunctionName(GlobalValue::GUID GUID) const {
auto It = FuncInfo.find(GUID);
@@ -81,22 +76,22 @@ class PGOContextualProfile {
uint32_t getNumCounters(const Function &F) const {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCounterIndex;
+ return FuncInfo.find(F.getGUID())->second.NextCounterIndex;
}
uint32_t getNumCallsites(const Function &F) const {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCallsiteIndex;
+ return FuncInfo.find(F.getGUID())->second.NextCallsiteIndex;
}
uint32_t allocateNextCounterIndex(const Function &F) {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCounterIndex++;
+ return FuncInfo.find(F.getGUID())->second.NextCounterIndex++;
}
uint32_t allocateNextCallsiteIndex(const Function &F) {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCallsiteIndex++;
+ return FuncInfo.find(F.getGUID())->second.NextCallsiteIndex++;
}
using ConstVisitor = function_ref<void(const PGOCtxProfContext &)>;
@@ -188,26 +183,5 @@ class ProfileAnnotator {
LLVM_ABI ~ProfileAnnotator();
};
-/// Assign a GUID to functions as metadata. GUID calculation takes linkage into
-/// account, which may change especially through and after thinlto. By
-/// pre-computing and assigning as metadata, this mechanism is resilient to such
-/// changes (as well as name changes e.g. suffix ".llvm." additions).
-
-// FIXME(mtrofin): we can generalize this mechanism to calculate a GUID early in
-// the pass pipeline, associate it with any Global Value, and then use it for
-// PGO and ThinLTO.
-// At that point, this should be moved elsewhere.
-class AssignGUIDPass : public PassInfoMixin<AssignGUIDPass> {
-public:
- explicit AssignGUIDPass() = default;
-
- /// Assign a GUID *if* one is not already assign, as a function metadata named
- /// `GUIDMetadataName`.
- LLVM_ABI PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM);
- LLVM_ABI static const char *GUIDMetadataName;
- // This should become GlobalValue::getGUID
- LLVM_ABI static uint64_t getGUID(const Function &F);
-};
-
} // namespace llvm
#endif // LLVM_ANALYSIS_CTXPROFANALYSIS_H
diff --git a/llvm/include/llvm/Bitcode/BitcodeReader.h b/llvm/include/llvm/Bitcode/BitcodeReader.h
index 772ca82019278..b7cab51857f0b 100644
--- a/llvm/include/llvm/Bitcode/BitcodeReader.h
+++ b/llvm/include/llvm/Bitcode/BitcodeReader.h
@@ -17,6 +17,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Bitstream/BitCodeEnums.h"
#include "llvm/IR/GlobalValue.h"
+#include "llvm/IR/ModuleSummaryIndex.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/Endian.h"
#include "llvm/Support/Error.h"
@@ -165,7 +166,8 @@ struct ParserCallbacks {
/// into CombinedIndex.
LLVM_ABI Error
readSummary(ModuleSummaryIndex &CombinedIndex, StringRef ModulePath,
- std::function<bool(GlobalValue::GUID)> IsPrevailing = nullptr);
+ std::function<bool(StringRef)> IsPrevailing = nullptr,
+ std::function<void(ValueInfo)> OnValueInfo = nullptr);
};
struct BitcodeFileContents {
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index bcf596a0d79b2..a3afad599186a 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -120,6 +120,9 @@ enum ModuleCodes {
// IFUNC: [ifunc value type, addrspace, resolver val#, linkage, visibility]
MODULE_CODE_IFUNC = 18,
+
+ // GUIDLIST: [n x i64]
+ MODULE_CODE_GUIDLIST = 19,
};
/// PARAMATTR blocks have code for defining a parameter attribute set.
diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def
index 0d79677d7079e..ced586aaa503e 100644
--- a/llvm/include/llvm/IR/FixedMetadataKinds.def
+++ b/llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -60,3 +60,4 @@ LLVM_FIXED_MD_KIND(MD_alloc_token, "alloc_token", 45)
LLVM_FIXED_MD_KIND(MD_implicit_ref, "implicit.ref", 46)
LLVM_FIXED_MD_KIND(MD_nofpclass, "nofpclass", 47)
LLVM_FIXED_MD_KIND(MD_call_target, "call_target", 48)
+LLVM_FIXED_MD_KIND(MD_unique_id, "guid", 49)
diff --git a/llvm/include/llvm/IR/GlobalValue.h b/llvm/include/llvm/IR/GlobalValue.h
index 83e695cdd27d9..acc836e1974d4 100644
--- a/llvm/include/llvm/IR/GlobalValue.h
+++ b/llvm/include/llvm/IR/GlobalValue.h
@@ -589,17 +589,40 @@ class GlobalValue : public Constant {
/// used as the key for a global lookup (e.g. profile or ThinLTO).
LLVM_ABI std::string getGlobalIdentifier() const;
+ /// Assign a GUID to this value based on its current name and linkage.
+ /// This GUID will remain the same even if those change. This method is
+ /// idempotent -- if a GUID has already been assigned, calling it again
+ /// will do nothing.
+ ///
+ /// This is private (exposed only to \c AssignGUIDPass), as users don't need
+ /// to call it. GUIDs are assigned only by \c AssignGUIDPass. The pass
+ /// pipeline should be set up such that GUIDs are always available when
+ /// needed. If not, the GUID assignment pass should be moved (or run again)
+ /// such that they are.
+ void assignGUID();
+
+ // assignGUID needs to be accessible from AssignGUIDPass, which is called
+ // early in the pipeline to make GUIDs available to later passes. But we'd
+ // rather not expose it publicly, as no-one else should call it.
+ friend class AssignGUIDPass;
+
public:
/// Return a 64-bit global unique ID constructed from the name of a global
/// symbol. Since this call doesn't supply the linkage or defining filename,
/// the GUID computation will assume that the global has external linkage.
LLVM_ABI static GUID getGUIDAssumingExternalLinkage(StringRef GlobalName);
- /// Return a 64-bit global unique ID constructed from global value name
- /// (i.e. returned by getGlobalIdentifier()).
- GUID getGUID() const {
- return getGUIDAssumingExternalLinkage(getGlobalIdentifier());
- }
+ /// Return a 64-bit global unique ID for this value. It is based on the
+ /// "original" name and linkage of this value (i.e. whenever its GUID was
+ /// assigned). This might not match the current name and linkage.
+ GUID getGUID() const;
+
+ /// Return the GUID for this value if it has been assigned, nullopt
+ /// otherwise. This should only need to be used in some exceptional
+ /// situations. If possible whatever code needs access to a GUID should
+ /// be set up to run after AssignGUIDPass, in which case it will always
+ /// be available.
+ std::optional<GUID> getGUIDIfAssigned() const;
/// @name Materialization
/// Materialization is used to construct functions only as they're needed.
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index 7156a83c9f3cc..b4a13128dd9f7 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -32,6 +32,7 @@
#include "llvm/Support/CBindingWrapping.h"
#include "llvm/Support/CodeGen.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
#include "llvm/TargetParser/Triple.h"
#include <cstddef>
#include <cstdint>
@@ -576,7 +577,22 @@ class LLVM_ABI Module {
// Use global_size() to get the total number of global variables.
// Use globals() to get the range of all global variables.
+ std::optional<GlobalValue::GUID> getGUID(const Value* V) const {
+ const auto It = ValueToGUIDMap.find(V);
+ if (It == ValueToGUIDMap.end()) {
+ return {};
+ }
+
+ return It->getSecond();
+ }
+
+ void insertGUID(const Value* V, GlobalValue::GUID GUID) {
+ ValueToGUIDMap[V] = GUID;
+ }
+
private:
+ DenseMap<const Value*, GlobalValue::GUID> ValueToGUIDMap;
+
/// @}
/// @name Direct access to the globals list, functions list, and symbol table
/// @{
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index b0b5ef9908a5e..07e019cbe48c4 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -29,6 +29,7 @@
#include "llvm/IR/Module.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
#include "llvm/Support/InterleavedRange.h"
#include "llvm/Support/ScaledNumber.h"
#include "llvm/Support/StringSaver.h"
@@ -599,8 +600,8 @@ class GlobalValueSummary {
bool wasPromoted() const { return Flags.Promoted; }
void promote() {
- assert(GlobalValue::isLocalLinkage(linkage()) &&
- "unexpected (re-)promotion of non-local symbol");
+ assert(!GlobalValue::isExternalLinkage(linkage()) &&
+ "unexpected (re-)promotion of external symbol");
assert(!Flags.Promoted);
Flags.Promoted = true;
Flags.Linkage = GlobalValue::LinkageTypes::ExternalLinkage;
@@ -1763,14 +1764,19 @@ class ModuleSummaryIndex {
return ValueInfo(HaveGVs, VP);
}
- /// Return a ValueInfo for \p GV and mark it as belonging to GV.
- ValueInfo getOrInsertValueInfo(const GlobalValue *GV) {
+ /// Return a ValueInfo for \p GV with GUID \p GUID and mark it as belonging to GV.
+ ValueInfo getOrInsertValueInfo(const GlobalValue *GV, GlobalValue::GUID GUID) {
assert(HaveGVs);
- auto VP = getOrInsertValuePtr(GV->getGUID());
+ auto VP = getOrInsertValuePtr(GUID);
VP->second.U.GV = GV;
return ValueInfo(HaveGVs, VP);
}
+ /// Return a ValueInfo for \p GV and mark it as belonging to GV.
+ ValueInfo getOrInsertValueInfo(const GlobalValue *GV) {
+ return getOrInsertValueInfo(GV, GV->getGUID());
+ }
+
/// Return the GUID for \p OriginalId in the OidGuidMap.
GlobalValue::GUID getGUIDFromOriginalID(GlobalValue::GUID OriginalID) const {
const auto I = OidGuidMap.find(OriginalID);
diff --git a/llvm/include/llvm/LTO/LTO.h b/llvm/include/llvm/LTO/LTO.h
index 38349b604b6b3..32a47b3c87e1d 100644
--- a/llvm/include/llvm/LTO/LTO.h
+++ b/llvm/include/llvm/LTO/LTO.h
@@ -572,6 +572,23 @@ class LTO {
/// LTO backend.
unsigned Partition = Unknown;
+ // FIXME: make private, and also make GlobalResolution a class, it has been
+ // becoming more than just a data bag.
+ GlobalValue::GUID GUID = 0;
+
+ void setGUID(GlobalValue::GUID G) {
+ assert(G);
+ assert(!GUID || GUID == G);
+ GUID = G;
+ }
+
+ GlobalValue::GUID getGUID() const {
+ return GUID ? GUID
+ : GlobalValue::getGUIDAssumingExternalLinkage(
+ GlobalValue::getGlobalIdentifier(
+ IRName, GlobalValue::LinkageTypes::ExternalLinkage,
+ ""));
+ }
/// Special partition numbers.
enum : unsigned {
/// A partition number has not yet been assigned to this global.
diff --git a/llvm/include/llvm/Transforms/Utils/AssignGUID.h b/llvm/include/llvm/Transforms/Utils/AssignGUID.h
new file mode 100644
index 0000000000000..0abd450d5a522
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Utils/AssignGUID.h
@@ -0,0 +1,42 @@
+//===-- AssignGUID.h - Unique identifier assignment pass --------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides a pass which assigns a a GUID (globally unique identifier)
+// to every GlobalValue in the module, according to its current name, linkage,
+// and originating file. This way we have a consistent identifier even when
+// these inputs to the GUID change (for instance, after externalising a global
+// in ThinLTO).
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
+#define LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
+
+#include "llvm/IR/Module.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Support/Debug.h"
+
+namespace llvm {
+
+class AssignGUIDPass : public PassInfoMixin<AssignGUIDPass> {
+public:
+ AssignGUIDPass() = default;
+
+ static void runOnModule(Module &M);
+
+ PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM) {
+ AssignGUIDPass::runOnModule(M);
+ return PreservedAnalyses::all();
+ }
+
+ static bool isRequired() { return true; }
+};
+
+} // end namespace llvm
+
+#endif // LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
\ No newline at end of file
diff --git a/llvm/lib/Analysis/CtxProfAnalysis.cpp b/llvm/lib/Analysis/CtxProfAnalysis.cpp
index c4abec02e765a..d4c00e4090220 100644
--- a/llvm/lib/Analysis/CtxProfAnalysis.cpp
+++ b/llvm/lib/Analysis/CtxProfAnalysis.cpp
@@ -50,9 +50,7 @@ static cl::opt<bool> ForceIsInSpecializedModule(
"ctx-profile-force-is-specialized", cl::init(false),
cl::desc("Treat the given module as-if it were containing the "
"post-thinlink module containing the root"));
-
-const char *AssignGUIDPass::GUIDMetadataName = "guid";
-
+
class ProfileAnnotatorImpl final {
friend class ProfileAnnotator;
class BBInfo;
@@ -420,33 +418,6 @@ bool ProfileAnnotator::getOutgoingBranchWeights(
return MaxCount > 0;
}
-PreservedAnalyses AssignGUIDPass::run(Module &M, ModuleAnalysisManager &MAM) {
- for (auto &F : M.functions()) {
- if (F.isDeclaration())
- continue;
- if (F.getMetadata(GUIDMetadataName))
- continue;
- const GlobalValue::GUID GUID = F.getGUID();
- F.setMetadata(GUIDMetadataName,
- MDNode::get(M.getContext(),
- {ConstantAsMetadata::get(ConstantInt::get(
- Type::getInt64Ty(M.getContext()), GUID))}));
- }
- return PreservedAnalyses::none();
-}
-
-GlobalValue::GUID AssignGUIDPass::getGUID(const Function &F) {
- if (F.isDeclaration()) {
- assert(GlobalValue::isExternalLinkage(F.getLinkage()));
- return F.getGUID();
- }
- auto *MD = F.getMetadata(GUIDMetadataName);
- assert(MD && "guid not found for defined function");
- return cast<ConstantInt>(cast<ConstantAsMetadata>(MD->getOperand(0))
- ->getValue()
- ->stripPointerCasts())
- ->getZExtValue();
-}
AnalysisKey CtxProfAnalysis::Key;
CtxProfAnalysis::CtxProfAnalysis(std::optional<StringRef> Profile)
@@ -515,7 +486,7 @@ PGOContextualProfile CtxProfAnalysis::run(Module &M,
for (const auto &F : M) {
if (F.isDeclaration())
continue;
- auto GUID = AssignGUIDPass::getGUID(F);
+ auto GUID = F.getGUID();
assert(GUID && "guid not found for defined function");
const auto &Entry = F.begin();
uint32_t MaxCounters = 0; // we expect at least a counter.
@@ -549,13 +520,6 @@ PGOContextualProfile CtxProfAnalysis::run(Module &M,
return Result;
}
-GlobalValue::GUID
-PGOContextualProfile::getDefinedFunctionGUID(const Function &F) const {
- if (auto It = FuncInfo.find(AssignGUIDPass::getGUID(F)); It != FuncInfo.end())
- return It->first;
- return 0;
-}
-
CtxProfAnalysisPrinterPass::CtxProfAnalysisPrinterPass(raw_ostream &OS)
: OS(OS), Mode(PrintLevel) {}
@@ -671,7 +635,7 @@ bool PGOContextualProfile::isInSpecializedModule() const {
void PGOContextualProfile::update(Visitor V, const Function &F) {
assert(isFunctionKnown(F));
- GlobalValue::GUID G = getDefinedFunctionGUID(F);
+ GlobalValue::GUID G = F.getGUID();
for (auto *Node = FuncInfo.find(G)->second.Index.Next; Node;
Node = Node->Next)
V(*reinterpret_cast<PGOCtxProfContext *>(Node));
@@ -682,7 +646,7 @@ void PGOContextualProfile::visit(ConstVisitor V, const Function *F) const {
return preorderVisit<const PGOCtxProfContext::CallTargetMapTy,
const PGOCtxProfContext>(Profiles.Contexts, V);
assert(isFunctionKnown(*F));
- GlobalValue::GUID G = getDefinedFunctionGUID(*F);
+ GlobalValue::GUID G = F->getGUID();
for (const auto *Node = FuncInfo.find(G)->second.Index.Next; Node;
Node = Node->Next)
V(*reinterpret_cast<const PGOCtxProfContext *>(Node));
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 551b88d8b435b..fcfa2ec2be342 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1064,8 +1064,9 @@ bool LLParser::skipModuleSummaryEntry() {
// support is in place we will look for the tokens corresponding to the
// expected tags.
if (Lex.getKind() != lltok::kw_gv && Lex.getKind() != lltok::kw_module &&
- Lex.getKind() != lltok::kw_typeid && Lex.getKind() != lltok::kw_flags &&
- Lex.getKind() != lltok::kw_blockcount)
+ Lex.getKind() != lltok::kw_typeid &&
+ Lex.getKind() != lltok::kw_typeidCompatibleVTable &&
+ Lex.getKind() != lltok::kw_flags && Lex.getKind() != lltok::kw_blockcount)
return tokError(
"Expected 'gv', 'module', 'typeid', 'flags' or 'blockcount' at the "
"start of summary entry");
@@ -9847,7 +9848,12 @@ bool LLParser::addGlobalValueToIndex(
if (!GV)
return error(Loc, "Reference to undefined global \"" + Name + "\"");
- VI = Index->getOrInsertValueInfo(GV);
+ // Be a little lenient here, to accomodate older files without GUIDs
+ // already computed and assigned as metadata.
+ auto MaybeGUID = GV->getGUIDIfAssigned();
+ GUID = MaybeGUID ? *MaybeGUID : GlobalValue::getGUIDAssumingExternalLinkage(GV->getName());
+
+ VI = Index->getOrInsertValueInfo(GV, GUID);
} else {
assert(
(!GlobalValue::isLocalLinkage(Linkage) || !SourceFileName.empty()) &&
diff --git a/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp b/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
index 911ec7501eb8b..6574ab7a93c58 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
@@ -142,6 +142,7 @@ GetCodeName(unsigned CodeID, unsigned BlockID,
STRINGIFY_CODE(MODULE_CODE, METADATA_VALUES_UNUSED)
STRINGIFY_CODE(MODULE_CODE, SOURCE_FILENAME)
STRINGIFY_CODE(MODULE_CODE, HASH)
+ STRINGIFY_CODE(MODULE_CODE, GUIDLIST)
}
case bitc::IDENTIFICATION_BLOCK_ID:
switch (CodeID) {
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index a781a43112f8c..0b6ce590f00af 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -696,6 +696,9 @@ class BitcodeReader : public BitcodeReaderBase, public GVMaterializer {
std::optional<ValueTypeCallbackTy> ValueTypeCallback;
+ /// A list of GUIDs defined by this module. Indexed by ValueID.
+ std::vector<GlobalValue::GUID> GUIDList;
+
public:
BitcodeReader(BitstreamCursor Stream, StringRef Strtab,
StringRef ProducerIdentification, LLVMContext &Context);
@@ -974,9 +977,10 @@ class ModuleSummaryIndexBitcodeReader : public BitcodeReaderBase {
/// Callback to ask whether a symbol is the prevailing copy when invoked
/// during combined index building.
- std::function<bool(GlobalValue::GUID)> IsPrevailing;
+ std::function<bool(StringRef)> IsPrevailing = nullptr;
+ std::function<void(ValueInfo)> OnValueInfo = nullptr;
- /// Saves the stack ids from the STACK_IDS record to consult when adding stack
+ /// Saves the stack ids from the STACK_IDS record to consult ...
[truncated]
|
Member
|
@llvm/pr-subscribers-llvm-analysis Author: Owen Rodley (orodley) ChangesThis allows us to keep GUIDs consistent across compilation phases which may change the name or linkage type. See https://discourse.llvm.org/t/rfc-keep-globalvalue-guids-stable/84801 Patch is 117.67 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/184065.diff 87 Files Affected:
diff --git a/llvm/include/llvm/Analysis/CtxProfAnalysis.h b/llvm/include/llvm/Analysis/CtxProfAnalysis.h
index 5c9823b0f6cc1..90b1d3fc0a688 100644
--- a/llvm/include/llvm/Analysis/CtxProfAnalysis.h
+++ b/llvm/include/llvm/Analysis/CtxProfAnalysis.h
@@ -47,9 +47,6 @@ class PGOContextualProfile {
// we'll need when we maintain the profiles during IPO transformations.
std::map<GlobalValue::GUID, FunctionInfo> FuncInfo;
- /// Get the GUID of this Function if it's defined in this module.
- LLVM_ABI GlobalValue::GUID getDefinedFunctionGUID(const Function &F) const;
-
// This is meant to be constructed from CtxProfAnalysis, which will also set
// its state piecemeal.
PGOContextualProfile() = default;
@@ -68,9 +65,7 @@ class PGOContextualProfile {
LLVM_ABI bool isInSpecializedModule() const;
- bool isFunctionKnown(const Function &F) const {
- return getDefinedFunctionGUID(F) != 0;
- }
+ bool isFunctionKnown(const Function &F) const { return !F.isDeclaration(); }
StringRef getFunctionName(GlobalValue::GUID GUID) const {
auto It = FuncInfo.find(GUID);
@@ -81,22 +76,22 @@ class PGOContextualProfile {
uint32_t getNumCounters(const Function &F) const {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCounterIndex;
+ return FuncInfo.find(F.getGUID())->second.NextCounterIndex;
}
uint32_t getNumCallsites(const Function &F) const {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCallsiteIndex;
+ return FuncInfo.find(F.getGUID())->second.NextCallsiteIndex;
}
uint32_t allocateNextCounterIndex(const Function &F) {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCounterIndex++;
+ return FuncInfo.find(F.getGUID())->second.NextCounterIndex++;
}
uint32_t allocateNextCallsiteIndex(const Function &F) {
assert(isFunctionKnown(F));
- return FuncInfo.find(getDefinedFunctionGUID(F))->second.NextCallsiteIndex++;
+ return FuncInfo.find(F.getGUID())->second.NextCallsiteIndex++;
}
using ConstVisitor = function_ref<void(const PGOCtxProfContext &)>;
@@ -188,26 +183,5 @@ class ProfileAnnotator {
LLVM_ABI ~ProfileAnnotator();
};
-/// Assign a GUID to functions as metadata. GUID calculation takes linkage into
-/// account, which may change especially through and after thinlto. By
-/// pre-computing and assigning as metadata, this mechanism is resilient to such
-/// changes (as well as name changes e.g. suffix ".llvm." additions).
-
-// FIXME(mtrofin): we can generalize this mechanism to calculate a GUID early in
-// the pass pipeline, associate it with any Global Value, and then use it for
-// PGO and ThinLTO.
-// At that point, this should be moved elsewhere.
-class AssignGUIDPass : public PassInfoMixin<AssignGUIDPass> {
-public:
- explicit AssignGUIDPass() = default;
-
- /// Assign a GUID *if* one is not already assign, as a function metadata named
- /// `GUIDMetadataName`.
- LLVM_ABI PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM);
- LLVM_ABI static const char *GUIDMetadataName;
- // This should become GlobalValue::getGUID
- LLVM_ABI static uint64_t getGUID(const Function &F);
-};
-
} // namespace llvm
#endif // LLVM_ANALYSIS_CTXPROFANALYSIS_H
diff --git a/llvm/include/llvm/Bitcode/BitcodeReader.h b/llvm/include/llvm/Bitcode/BitcodeReader.h
index 772ca82019278..b7cab51857f0b 100644
--- a/llvm/include/llvm/Bitcode/BitcodeReader.h
+++ b/llvm/include/llvm/Bitcode/BitcodeReader.h
@@ -17,6 +17,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Bitstream/BitCodeEnums.h"
#include "llvm/IR/GlobalValue.h"
+#include "llvm/IR/ModuleSummaryIndex.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/Endian.h"
#include "llvm/Support/Error.h"
@@ -165,7 +166,8 @@ struct ParserCallbacks {
/// into CombinedIndex.
LLVM_ABI Error
readSummary(ModuleSummaryIndex &CombinedIndex, StringRef ModulePath,
- std::function<bool(GlobalValue::GUID)> IsPrevailing = nullptr);
+ std::function<bool(StringRef)> IsPrevailing = nullptr,
+ std::function<void(ValueInfo)> OnValueInfo = nullptr);
};
struct BitcodeFileContents {
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index bcf596a0d79b2..a3afad599186a 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -120,6 +120,9 @@ enum ModuleCodes {
// IFUNC: [ifunc value type, addrspace, resolver val#, linkage, visibility]
MODULE_CODE_IFUNC = 18,
+
+ // GUIDLIST: [n x i64]
+ MODULE_CODE_GUIDLIST = 19,
};
/// PARAMATTR blocks have code for defining a parameter attribute set.
diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def
index 0d79677d7079e..ced586aaa503e 100644
--- a/llvm/include/llvm/IR/FixedMetadataKinds.def
+++ b/llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -60,3 +60,4 @@ LLVM_FIXED_MD_KIND(MD_alloc_token, "alloc_token", 45)
LLVM_FIXED_MD_KIND(MD_implicit_ref, "implicit.ref", 46)
LLVM_FIXED_MD_KIND(MD_nofpclass, "nofpclass", 47)
LLVM_FIXED_MD_KIND(MD_call_target, "call_target", 48)
+LLVM_FIXED_MD_KIND(MD_unique_id, "guid", 49)
diff --git a/llvm/include/llvm/IR/GlobalValue.h b/llvm/include/llvm/IR/GlobalValue.h
index 83e695cdd27d9..acc836e1974d4 100644
--- a/llvm/include/llvm/IR/GlobalValue.h
+++ b/llvm/include/llvm/IR/GlobalValue.h
@@ -589,17 +589,40 @@ class GlobalValue : public Constant {
/// used as the key for a global lookup (e.g. profile or ThinLTO).
LLVM_ABI std::string getGlobalIdentifier() const;
+ /// Assign a GUID to this value based on its current name and linkage.
+ /// This GUID will remain the same even if those change. This method is
+ /// idempotent -- if a GUID has already been assigned, calling it again
+ /// will do nothing.
+ ///
+ /// This is private (exposed only to \c AssignGUIDPass), as users don't need
+ /// to call it. GUIDs are assigned only by \c AssignGUIDPass. The pass
+ /// pipeline should be set up such that GUIDs are always available when
+ /// needed. If not, the GUID assignment pass should be moved (or run again)
+ /// such that they are.
+ void assignGUID();
+
+ // assignGUID needs to be accessible from AssignGUIDPass, which is called
+ // early in the pipeline to make GUIDs available to later passes. But we'd
+ // rather not expose it publicly, as no-one else should call it.
+ friend class AssignGUIDPass;
+
public:
/// Return a 64-bit global unique ID constructed from the name of a global
/// symbol. Since this call doesn't supply the linkage or defining filename,
/// the GUID computation will assume that the global has external linkage.
LLVM_ABI static GUID getGUIDAssumingExternalLinkage(StringRef GlobalName);
- /// Return a 64-bit global unique ID constructed from global value name
- /// (i.e. returned by getGlobalIdentifier()).
- GUID getGUID() const {
- return getGUIDAssumingExternalLinkage(getGlobalIdentifier());
- }
+ /// Return a 64-bit global unique ID for this value. It is based on the
+ /// "original" name and linkage of this value (i.e. whenever its GUID was
+ /// assigned). This might not match the current name and linkage.
+ GUID getGUID() const;
+
+ /// Return the GUID for this value if it has been assigned, nullopt
+ /// otherwise. This should only need to be used in some exceptional
+ /// situations. If possible whatever code needs access to a GUID should
+ /// be set up to run after AssignGUIDPass, in which case it will always
+ /// be available.
+ std::optional<GUID> getGUIDIfAssigned() const;
/// @name Materialization
/// Materialization is used to construct functions only as they're needed.
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index 7156a83c9f3cc..b4a13128dd9f7 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -32,6 +32,7 @@
#include "llvm/Support/CBindingWrapping.h"
#include "llvm/Support/CodeGen.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
#include "llvm/TargetParser/Triple.h"
#include <cstddef>
#include <cstdint>
@@ -576,7 +577,22 @@ class LLVM_ABI Module {
// Use global_size() to get the total number of global variables.
// Use globals() to get the range of all global variables.
+ std::optional<GlobalValue::GUID> getGUID(const Value* V) const {
+ const auto It = ValueToGUIDMap.find(V);
+ if (It == ValueToGUIDMap.end()) {
+ return {};
+ }
+
+ return It->getSecond();
+ }
+
+ void insertGUID(const Value* V, GlobalValue::GUID GUID) {
+ ValueToGUIDMap[V] = GUID;
+ }
+
private:
+ DenseMap<const Value*, GlobalValue::GUID> ValueToGUIDMap;
+
/// @}
/// @name Direct access to the globals list, functions list, and symbol table
/// @{
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index b0b5ef9908a5e..07e019cbe48c4 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -29,6 +29,7 @@
#include "llvm/IR/Module.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
#include "llvm/Support/InterleavedRange.h"
#include "llvm/Support/ScaledNumber.h"
#include "llvm/Support/StringSaver.h"
@@ -599,8 +600,8 @@ class GlobalValueSummary {
bool wasPromoted() const { return Flags.Promoted; }
void promote() {
- assert(GlobalValue::isLocalLinkage(linkage()) &&
- "unexpected (re-)promotion of non-local symbol");
+ assert(!GlobalValue::isExternalLinkage(linkage()) &&
+ "unexpected (re-)promotion of external symbol");
assert(!Flags.Promoted);
Flags.Promoted = true;
Flags.Linkage = GlobalValue::LinkageTypes::ExternalLinkage;
@@ -1763,14 +1764,19 @@ class ModuleSummaryIndex {
return ValueInfo(HaveGVs, VP);
}
- /// Return a ValueInfo for \p GV and mark it as belonging to GV.
- ValueInfo getOrInsertValueInfo(const GlobalValue *GV) {
+ /// Return a ValueInfo for \p GV with GUID \p GUID and mark it as belonging to GV.
+ ValueInfo getOrInsertValueInfo(const GlobalValue *GV, GlobalValue::GUID GUID) {
assert(HaveGVs);
- auto VP = getOrInsertValuePtr(GV->getGUID());
+ auto VP = getOrInsertValuePtr(GUID);
VP->second.U.GV = GV;
return ValueInfo(HaveGVs, VP);
}
+ /// Return a ValueInfo for \p GV and mark it as belonging to GV.
+ ValueInfo getOrInsertValueInfo(const GlobalValue *GV) {
+ return getOrInsertValueInfo(GV, GV->getGUID());
+ }
+
/// Return the GUID for \p OriginalId in the OidGuidMap.
GlobalValue::GUID getGUIDFromOriginalID(GlobalValue::GUID OriginalID) const {
const auto I = OidGuidMap.find(OriginalID);
diff --git a/llvm/include/llvm/LTO/LTO.h b/llvm/include/llvm/LTO/LTO.h
index 38349b604b6b3..32a47b3c87e1d 100644
--- a/llvm/include/llvm/LTO/LTO.h
+++ b/llvm/include/llvm/LTO/LTO.h
@@ -572,6 +572,23 @@ class LTO {
/// LTO backend.
unsigned Partition = Unknown;
+ // FIXME: make private, and also make GlobalResolution a class, it has been
+ // becoming more than just a data bag.
+ GlobalValue::GUID GUID = 0;
+
+ void setGUID(GlobalValue::GUID G) {
+ assert(G);
+ assert(!GUID || GUID == G);
+ GUID = G;
+ }
+
+ GlobalValue::GUID getGUID() const {
+ return GUID ? GUID
+ : GlobalValue::getGUIDAssumingExternalLinkage(
+ GlobalValue::getGlobalIdentifier(
+ IRName, GlobalValue::LinkageTypes::ExternalLinkage,
+ ""));
+ }
/// Special partition numbers.
enum : unsigned {
/// A partition number has not yet been assigned to this global.
diff --git a/llvm/include/llvm/Transforms/Utils/AssignGUID.h b/llvm/include/llvm/Transforms/Utils/AssignGUID.h
new file mode 100644
index 0000000000000..0abd450d5a522
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Utils/AssignGUID.h
@@ -0,0 +1,42 @@
+//===-- AssignGUID.h - Unique identifier assignment pass --------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides a pass which assigns a a GUID (globally unique identifier)
+// to every GlobalValue in the module, according to its current name, linkage,
+// and originating file. This way we have a consistent identifier even when
+// these inputs to the GUID change (for instance, after externalising a global
+// in ThinLTO).
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
+#define LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
+
+#include "llvm/IR/Module.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Support/Debug.h"
+
+namespace llvm {
+
+class AssignGUIDPass : public PassInfoMixin<AssignGUIDPass> {
+public:
+ AssignGUIDPass() = default;
+
+ static void runOnModule(Module &M);
+
+ PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM) {
+ AssignGUIDPass::runOnModule(M);
+ return PreservedAnalyses::all();
+ }
+
+ static bool isRequired() { return true; }
+};
+
+} // end namespace llvm
+
+#endif // LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
\ No newline at end of file
diff --git a/llvm/lib/Analysis/CtxProfAnalysis.cpp b/llvm/lib/Analysis/CtxProfAnalysis.cpp
index c4abec02e765a..d4c00e4090220 100644
--- a/llvm/lib/Analysis/CtxProfAnalysis.cpp
+++ b/llvm/lib/Analysis/CtxProfAnalysis.cpp
@@ -50,9 +50,7 @@ static cl::opt<bool> ForceIsInSpecializedModule(
"ctx-profile-force-is-specialized", cl::init(false),
cl::desc("Treat the given module as-if it were containing the "
"post-thinlink module containing the root"));
-
-const char *AssignGUIDPass::GUIDMetadataName = "guid";
-
+
class ProfileAnnotatorImpl final {
friend class ProfileAnnotator;
class BBInfo;
@@ -420,33 +418,6 @@ bool ProfileAnnotator::getOutgoingBranchWeights(
return MaxCount > 0;
}
-PreservedAnalyses AssignGUIDPass::run(Module &M, ModuleAnalysisManager &MAM) {
- for (auto &F : M.functions()) {
- if (F.isDeclaration())
- continue;
- if (F.getMetadata(GUIDMetadataName))
- continue;
- const GlobalValue::GUID GUID = F.getGUID();
- F.setMetadata(GUIDMetadataName,
- MDNode::get(M.getContext(),
- {ConstantAsMetadata::get(ConstantInt::get(
- Type::getInt64Ty(M.getContext()), GUID))}));
- }
- return PreservedAnalyses::none();
-}
-
-GlobalValue::GUID AssignGUIDPass::getGUID(const Function &F) {
- if (F.isDeclaration()) {
- assert(GlobalValue::isExternalLinkage(F.getLinkage()));
- return F.getGUID();
- }
- auto *MD = F.getMetadata(GUIDMetadataName);
- assert(MD && "guid not found for defined function");
- return cast<ConstantInt>(cast<ConstantAsMetadata>(MD->getOperand(0))
- ->getValue()
- ->stripPointerCasts())
- ->getZExtValue();
-}
AnalysisKey CtxProfAnalysis::Key;
CtxProfAnalysis::CtxProfAnalysis(std::optional<StringRef> Profile)
@@ -515,7 +486,7 @@ PGOContextualProfile CtxProfAnalysis::run(Module &M,
for (const auto &F : M) {
if (F.isDeclaration())
continue;
- auto GUID = AssignGUIDPass::getGUID(F);
+ auto GUID = F.getGUID();
assert(GUID && "guid not found for defined function");
const auto &Entry = F.begin();
uint32_t MaxCounters = 0; // we expect at least a counter.
@@ -549,13 +520,6 @@ PGOContextualProfile CtxProfAnalysis::run(Module &M,
return Result;
}
-GlobalValue::GUID
-PGOContextualProfile::getDefinedFunctionGUID(const Function &F) const {
- if (auto It = FuncInfo.find(AssignGUIDPass::getGUID(F)); It != FuncInfo.end())
- return It->first;
- return 0;
-}
-
CtxProfAnalysisPrinterPass::CtxProfAnalysisPrinterPass(raw_ostream &OS)
: OS(OS), Mode(PrintLevel) {}
@@ -671,7 +635,7 @@ bool PGOContextualProfile::isInSpecializedModule() const {
void PGOContextualProfile::update(Visitor V, const Function &F) {
assert(isFunctionKnown(F));
- GlobalValue::GUID G = getDefinedFunctionGUID(F);
+ GlobalValue::GUID G = F.getGUID();
for (auto *Node = FuncInfo.find(G)->second.Index.Next; Node;
Node = Node->Next)
V(*reinterpret_cast<PGOCtxProfContext *>(Node));
@@ -682,7 +646,7 @@ void PGOContextualProfile::visit(ConstVisitor V, const Function *F) const {
return preorderVisit<const PGOCtxProfContext::CallTargetMapTy,
const PGOCtxProfContext>(Profiles.Contexts, V);
assert(isFunctionKnown(*F));
- GlobalValue::GUID G = getDefinedFunctionGUID(*F);
+ GlobalValue::GUID G = F->getGUID();
for (const auto *Node = FuncInfo.find(G)->second.Index.Next; Node;
Node = Node->Next)
V(*reinterpret_cast<const PGOCtxProfContext *>(Node));
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 551b88d8b435b..fcfa2ec2be342 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1064,8 +1064,9 @@ bool LLParser::skipModuleSummaryEntry() {
// support is in place we will look for the tokens corresponding to the
// expected tags.
if (Lex.getKind() != lltok::kw_gv && Lex.getKind() != lltok::kw_module &&
- Lex.getKind() != lltok::kw_typeid && Lex.getKind() != lltok::kw_flags &&
- Lex.getKind() != lltok::kw_blockcount)
+ Lex.getKind() != lltok::kw_typeid &&
+ Lex.getKind() != lltok::kw_typeidCompatibleVTable &&
+ Lex.getKind() != lltok::kw_flags && Lex.getKind() != lltok::kw_blockcount)
return tokError(
"Expected 'gv', 'module', 'typeid', 'flags' or 'blockcount' at the "
"start of summary entry");
@@ -9847,7 +9848,12 @@ bool LLParser::addGlobalValueToIndex(
if (!GV)
return error(Loc, "Reference to undefined global \"" + Name + "\"");
- VI = Index->getOrInsertValueInfo(GV);
+ // Be a little lenient here, to accomodate older files without GUIDs
+ // already computed and assigned as metadata.
+ auto MaybeGUID = GV->getGUIDIfAssigned();
+ GUID = MaybeGUID ? *MaybeGUID : GlobalValue::getGUIDAssumingExternalLinkage(GV->getName());
+
+ VI = Index->getOrInsertValueInfo(GV, GUID);
} else {
assert(
(!GlobalValue::isLocalLinkage(Linkage) || !SourceFileName.empty()) &&
diff --git a/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp b/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
index 911ec7501eb8b..6574ab7a93c58 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
@@ -142,6 +142,7 @@ GetCodeName(unsigned CodeID, unsigned BlockID,
STRINGIFY_CODE(MODULE_CODE, METADATA_VALUES_UNUSED)
STRINGIFY_CODE(MODULE_CODE, SOURCE_FILENAME)
STRINGIFY_CODE(MODULE_CODE, HASH)
+ STRINGIFY_CODE(MODULE_CODE, GUIDLIST)
}
case bitc::IDENTIFICATION_BLOCK_ID:
switch (CodeID) {
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index a781a43112f8c..0b6ce590f00af 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -696,6 +696,9 @@ class BitcodeReader : public BitcodeReaderBase, public GVMaterializer {
std::optional<ValueTypeCallbackTy> ValueTypeCallback;
+ /// A list of GUIDs defined by this module. Indexed by ValueID.
+ std::vector<GlobalValue::GUID> GUIDList;
+
public:
BitcodeReader(BitstreamCursor Stream, StringRef Strtab,
StringRef ProducerIdentification, LLVMContext &Context);
@@ -974,9 +977,10 @@ class ModuleSummaryIndexBitcodeReader : public BitcodeReaderBase {
/// Callback to ask whether a symbol is the prevailing copy when invoked
/// during combined index building.
- std::function<bool(GlobalValue::GUID)> IsPrevailing;
+ std::function<bool(StringRef)> IsPrevailing = nullptr;
+ std::function<void(ValueInfo)> OnValueInfo = nullptr;
- /// Saves the stack ids from the STACK_IDS record to consult when adding stack
+ /// Saves the stack ids from the STACK_IDS record to consult ...
[truncated]
|
Contributor
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
Sorry for the slow review. A bunch of misc mostly minor questions and comments.
This allows us to keep GUIDs consistent across compilation phases which may change the name or linkage type. See https://discourse.llvm.org/t/rfc-keep-globalvalue-guids-stable/84801
Member
If it helps: do you also have a custom pass pipeline? If the issue is an assert about missing guid metadata, you may just need to run the |
Contributor
|
It looks like this causes a compile-time regression, esp. for ThinLTO: https://llvm-compile-time-tracker.com/compare.php?from=e0ef143823ce8f733c50fae675f05d0df0f702e5&to=7938535a54b51a4ca84821b7766d68049c9aa895&stat=instructions:u Can you please investigate what the cause of the issue is? This doesn't seem like a change that should impact compile-time if implemented correctly. |
Contributor
|
I'm also seeing this cause global merging to no longer occur. tmp.c Compiled with Running without the guid metadata the output for fn is |
Contributor
|
Looks like this is probably due to this bit in GlobalMergeImpl::run in llvm/lib/CodeGen/GlobalMerge.cpp |
Member
|
Looking at both today (and thanks @john-brawn-arm for the pointer!) |
Contributor
|
This is causing quite a few clang crashes with internal google code that are built with fdo and thinlto enabled. We don't have a reproducer yet that can be shared externally but here is the modified crash stack trace when running with clang-checked. |
Member
I was able to repro for the file causing the largest relative regression - libclamav_entconv (1.67% more instructions retired), in the ReleaseThinLTO config. Turns out it has lots of globals, and just 7, not particularly large, functions. The result is a large number of global values to write to the global summary, and also lots of guids. The function bodies aren't particularly large, though, so the extra bitcode writing because of the guids is detectable. |
mtrofin
added a commit
that referenced
this pull request
May 29, 2026
Follow up from PR #184065. Since globals or constants would now have their GUID pre-calculated and affixed with metadata, we need to tolerate its presence when merging. To that effect, we rename (and change accordingly) `GlobalObject::hasMetadataOtherThanDebugLoc` to `...AndGuid`. In the case of Constants, they are merged into one of them, which then keeps its guid, while the rest are erased. In the case of GlobalVariables, a new one is created, and we will give it its own guid.
|
This patch has resulted in the following appearing for me at link time: ptr @_Z4set1Pi |
Contributor
This needs a reproducer. |
We had not applied #200323 yet, so this warning may already be gone. |
Contributor
I don't think this is the whole story. This causes a ~0.3% regression during the thin link of clang (and llvm-tblgen / clang-tblgen), and those are certainly not workloads dominated by globals. This seems like a pretty across the board regression for ThinLTO. |
Member
It would be visible during ThinTLO mainly, since that's when we'd write summaries which now contain an extra table, but I agree that confirming that's all we're seeing is a good idea. How do the clang builds get measured by the compile time tracker, |
Contributor
CTT I believe uses |
Contributor
|
How this could work? Madata is unreliable, CFI requires reliable GUIDS. |
Contributor
|
We should revert and follow up with issues. BTW. It would be easier to deal with regressions if it was landed as set of smaller patches according to https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity. |
Member
There have already been some preparatory patches. The bulk of the patch is test changes due to tests assuming stuff like the last character in a declaration is |
Member
The RFC (linked in the patch) details this more, this patch actually makes GUIDs and their presence by globals more reliable rather than less. Before, with the on-the-fly name/linkage based computation, a lot of complexity was required to handle things like linkage changes and name suffixes (see the RFC) |
mtrofin
added a commit
to mtrofin/llvm-project
that referenced
this pull request
May 30, 2026
LowerTypeTests and the ModuleSummaryIndex mapped CFI function declarations and definitions based purely on the string name. If an internal function was promoted and renamed during ThinLTO, its name-derived GUID would change, leading to mismatches, resulting in runtime traps. This patch saves the GUIDs in the CFI tables. LowerTypeTests doesn't need to perform name-based lookups, relying on this GUID instead.
zengshzh
pushed a commit
to zengshzh/llvm-project
that referenced
this pull request
May 30, 2026
This allows us to keep GUIDs consistent across compilation phases which may change the name or linkage type. See https://discourse.llvm.org/t/rfc-keep-globalvalue-guids-stable/84801 This is a large change since the addition of metadata breaks many tests. The test changes are mostly just trivial changes to checks to get them passing.
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 1, 2026
LLVM 23: Run AssignGUIDPass in some places llvm/llvm-project#184065 introduced the new pass. I'm not 100% confident that this is the correct patch, but it makes all the previously failing `run-make` "lto" tests pass on my machine. I added the pass to the `if (NeedThinLTOBufferPasses)` section to match the `addRequiredLTOPreLinkPasses` changes from the LLVM PR, and to the `LLVMRustModuleSerialize` section based on the stack trace of an LLVM assertion. The IR may now contain an additional `!guid` attribute, requiring some codegen test expectations to be updated. I mostly followed the pattern from the LLVM PR, and removed trailing `{`s. (For both, tests that were failing locally, as well as potentially problematic expectations identified via grep, in case the test wasn't being run locally). @rustbot label llvm-main
rust-timer
added a commit
to rust-lang/rust
that referenced
this pull request
Jun 2, 2026
Rollup merge of #157055 - TimNN:guid, r=cuviper LLVM 23: Run AssignGUIDPass in some places llvm/llvm-project#184065 introduced the new pass. I'm not 100% confident that this is the correct patch, but it makes all the previously failing `run-make` "lto" tests pass on my machine. I added the pass to the `if (NeedThinLTOBufferPasses)` section to match the `addRequiredLTOPreLinkPasses` changes from the LLVM PR, and to the `LLVMRustModuleSerialize` section based on the stack trace of an LLVM assertion. The IR may now contain an additional `!guid` attribute, requiring some codegen test expectations to be updated. I mostly followed the pattern from the LLVM PR, and removed trailing `{`s. (For both, tests that were failing locally, as well as potentially problematic expectations identified via grep, in case the test wasn't being run locally). @rustbot label llvm-main
pull Bot
pushed a commit
to xtqqczze/rust-lang-miri
that referenced
this pull request
Jun 2, 2026
LLVM 23: Run AssignGUIDPass in some places llvm/llvm-project#184065 introduced the new pass. I'm not 100% confident that this is the correct patch, but it makes all the previously failing `run-make` "lto" tests pass on my machine. I added the pass to the `if (NeedThinLTOBufferPasses)` section to match the `addRequiredLTOPreLinkPasses` changes from the LLVM PR, and to the `LLVMRustModuleSerialize` section based on the stack trace of an LLVM assertion. The IR may now contain an additional `!guid` attribute, requiring some codegen test expectations to be updated. I mostly followed the pattern from the LLVM PR, and removed trailing `{`s. (For both, tests that were failing locally, as well as potentially problematic expectations identified via grep, in case the test wasn't being run locally). @rustbot label llvm-main
Contributor
|
We're hitting CFI failures after this (https://crbug.com/519012435). I see Vitaly already asked for it to be reverted. Can we revert back to green? |
mtrofin
added a commit
that referenced
this pull request
Jun 2, 2026
…201194) There is a relation between CFI and ThinLTO GUIDs that still needs to be disentangled first. Note that we leave the `MD_unique_id` in `FixedMetadataKinds.def` to avoid needing to re-number it later. Plus the metadata string ("guid") itself is used by ctxprof.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows us to keep GUIDs consistent across compilation phases which may change the name or linkage type.
See https://discourse.llvm.org/t/rfc-keep-globalvalue-guids-stable/84801