Ignore !guid metadata when merging globals or constants#200323
Merged
Conversation
Follow up from PR llvm#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.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Mircea Trofin (mtrofin) ChangesFollow 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) 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. Full diff: https://github.com/llvm/llvm-project/pull/200323.diff 8 Files Affected:
diff --git a/llvm/include/llvm/IR/GlobalObject.h b/llvm/include/llvm/IR/GlobalObject.h
index 1e03e22f0403b..746ccf5ecc994 100644
--- a/llvm/include/llvm/IR/GlobalObject.h
+++ b/llvm/include/llvm/IR/GlobalObject.h
@@ -181,7 +181,7 @@ class GlobalObject : public GlobalValue {
SmallVectorImpl<MDNode *> &MDs) const;
/// @}
- LLVM_ABI bool hasMetadataOtherThanDebugLoc() const;
+ LLVM_ABI bool hasMetadataOtherThanDebugLocAndGuid() const;
/// Copy metadata from Src, adjusting offsets by Offset.
LLVM_ABI void copyMetadata(const GlobalObject *Src, unsigned Offset);
diff --git a/llvm/include/llvm/Transforms/Utils/AssignGUID.h b/llvm/include/llvm/Transforms/Utils/AssignGUID.h
index 0abd450d5a522..1b2d718681da8 100644
--- a/llvm/include/llvm/Transforms/Utils/AssignGUID.h
+++ b/llvm/include/llvm/Transforms/Utils/AssignGUID.h
@@ -17,6 +17,8 @@
#ifndef LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
#define LLVM_TRANSFORMS_UTILS_ASSIGNGUID_H
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/PassManager.h"
#include "llvm/Support/Debug.h"
@@ -35,6 +37,11 @@ class AssignGUIDPass : public PassInfoMixin<AssignGUIDPass> {
}
static bool isRequired() { return true; }
+
+ // Let GlobalMerge assign a GUID for merged GVs, instead of needing to
+ // traverse all the module; or instead of making GlobalValue::assignGUID
+ // public.
+ static void assignGUIDForMergedGV(GlobalVariable &GV);
};
} // end namespace llvm
diff --git a/llvm/lib/CodeGen/GlobalMerge.cpp b/llvm/lib/CodeGen/GlobalMerge.cpp
index d52706b5f9bef..152ad43bc77bd 100644
--- a/llvm/lib/CodeGen/GlobalMerge.cpp
+++ b/llvm/lib/CodeGen/GlobalMerge.cpp
@@ -80,6 +80,7 @@
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/Type.h"
#include "llvm/IR/Use.h"
@@ -94,6 +95,7 @@
#include "llvm/Target/TargetLoweringObjectFile.h"
#include "llvm/Target/TargetMachine.h"
#include "llvm/TargetParser/Triple.h"
+#include "llvm/Transforms/Utils/AssignGUID.h"
#include <algorithm>
#include <cassert>
#include <cstddef>
@@ -605,6 +607,7 @@ bool GlobalMergeImpl::doMerge(const SmallVectorImpl<GlobalVariable *> &Globals,
NumMerged++;
}
+ AssignGUIDPass::assignGUIDForMergedGV(*MergedGV);
Changed = true;
i = j;
}
@@ -733,7 +736,7 @@ bool GlobalMergeImpl::run(Module &M) {
// component globals to the combined global, which only knows how to do this
// correctly for !dbg (and !type, but by this point LowerTypeTests will have
// already run).
- if (GV.hasMetadataOtherThanDebugLoc())
+ if (GV.hasMetadataOtherThanDebugLocAndGuid())
continue;
Type *Ty = GV.getValueType();
diff --git a/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp
index 27e934369b285..832eed31fa40a 100644
--- a/llvm/lib/IR/Globals.cpp
+++ b/llvm/lib/IR/Globals.cpp
@@ -452,11 +452,11 @@ bool GlobalObject::canIncreaseAlignment() const {
return true;
}
-bool GlobalObject::hasMetadataOtherThanDebugLoc() const {
+bool GlobalObject::hasMetadataOtherThanDebugLocAndGuid() const {
SmallVector<std::pair<unsigned, MDNode *>, 4> MDs;
getAllMetadata(MDs);
for (const auto &V : MDs)
- if (V.first != LLVMContext::MD_dbg)
+ if (V.first != LLVMContext::MD_dbg && V.first != LLVMContext::MD_unique_id)
return true;
return false;
}
diff --git a/llvm/lib/Transforms/IPO/ConstantMerge.cpp b/llvm/lib/Transforms/IPO/ConstantMerge.cpp
index 06e684dbe5a93..426c24e30c622 100644
--- a/llvm/lib/Transforms/IPO/ConstantMerge.cpp
+++ b/llvm/lib/Transforms/IPO/ConstantMerge.cpp
@@ -98,9 +98,9 @@ enum class CanMerge { No, Yes };
static CanMerge makeMergeable(GlobalVariable *Old, GlobalVariable *New) {
if (!Old->hasGlobalUnnamedAddr() && !New->hasGlobalUnnamedAddr())
return CanMerge::No;
- if (Old->hasMetadataOtherThanDebugLoc())
+ if (Old->hasMetadataOtherThanDebugLocAndGuid())
return CanMerge::No;
- assert(!New->hasMetadataOtherThanDebugLoc());
+ assert(!New->hasMetadataOtherThanDebugLocAndGuid());
// Merging constants with different comdats means one group cannot in general
// be dropped independently without the other group now having an invalid
@@ -176,7 +176,7 @@ static bool mergeConstants(Module &M) {
continue;
// Don't touch globals with metadata other then !dbg.
- if (GV.hasMetadataOtherThanDebugLoc())
+ if (GV.hasMetadataOtherThanDebugLocAndGuid())
continue;
Constant *Init = GV.getInitializer();
diff --git a/llvm/lib/Transforms/Utils/AssignGUID.cpp b/llvm/lib/Transforms/Utils/AssignGUID.cpp
index eb2c4844c1c87..b05504ad7181a 100644
--- a/llvm/lib/Transforms/Utils/AssignGUID.cpp
+++ b/llvm/lib/Transforms/Utils/AssignGUID.cpp
@@ -29,4 +29,13 @@ void AssignGUIDPass::runOnModule(Module &M) {
continue;
F.assignGUID();
}
-}
\ No newline at end of file
+}
+
+void AssignGUIDPass::assignGUIDForMergedGV(GlobalVariable &GV) {
+ // FIXME: merging adds all the guids of the original GVs. We currently drop
+ // that metadata from GV first, but we may want to remember those later, if
+ // we had a motivation for that. In that case, we need some other metadata
+ // to maintain that association.
+ GV.eraseMetadata(LLVMContext::MD_unique_id);
+ GV.assignGUID();
+}
diff --git a/llvm/test/Transforms/ConstantMerge/merge-dbg.ll b/llvm/test/Transforms/ConstantMerge/merge-dbg.ll
index de83ffbbda058..27b9101b018ff 100644
--- a/llvm/test/Transforms/ConstantMerge/merge-dbg.ll
+++ b/llvm/test/Transforms/ConstantMerge/merge-dbg.ll
@@ -1,4 +1,5 @@
; RUN: opt < %s -passes=constmerge -S | FileCheck %s
+; RUN: opt < %s -passes=assign-guid,constmerge -S | FileCheck %s
; CHECK: = constant i32 1, !dbg [[A:![0-9]+]], !dbg [[B:![0-9]+]]
@a = internal constant i32 1, !dbg !0
diff --git a/llvm/test/Transforms/GlobalMerge/guid.ll b/llvm/test/Transforms/GlobalMerge/guid.ll
new file mode 100644
index 0000000000000..01a5994bce48c
--- /dev/null
+++ b/llvm/test/Transforms/GlobalMerge/guid.ll
@@ -0,0 +1,38 @@
+; We can merge globals with a guid metadata, and the guid of the merged object
+; is different from that of the original objects.
+; RUN: opt -passes='global-merge<max-offset=100>' -S -o - %s | FileCheck %s
+
+target datalayout = "e-p:64:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: @_MergedGlobals = private global <{ i32, i32 }> <{ i32 1, i32 2 }>, align 4, !guid !0
+; CHECK: @_MergedGlobals.1 = private global <{ i32, i32 }> <{ i32 3, i32 4 }>, section "foo", align 4, !guid !1
+
+; CHECK-DAG: @a = internal alias i32, ptr @_MergedGlobals{{$}}
+@a = internal global i32 1, !guid !{i64 1}
+
+; CHECK-DAG: @b = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, ptr @_MergedGlobals, i32 0, i32 1)
+@b = internal global i32 2, !guid !{i64 2}
+
+; CHECK-DAG: @c = internal alias i32, ptr @_MergedGlobals.1{{$}}
+@c = internal global i32 3, section "foo", !guid !{i64 3}
+
+; CHECK-DAG: @d = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, ptr @_MergedGlobals.1, i32 0, i32 1)
+@d = internal global i32 4, section "foo", !guid !{i64 4}
+
+; CHECK: define void @use() !guid !2
+define void @use() !guid !{i64 5} {
+ ; CHECK: load i32, ptr @_MergedGlobals,
+ %x = load i32, ptr @a
+ ; CHECK: load i32, ptr getelementptr inbounds (<{ i32, i32 }>, ptr @_MergedGlobals, i32 0, i32 1)
+ %y = load i32, ptr @b
+ ; CHECK: load i32, ptr @_MergedGlobals.1
+ %z1 = load i32, ptr @c
+ ; CHECK: load i32, ptr getelementptr inbounds (<{ i32, i32 }>, ptr @_MergedGlobals.1, i32 0, i32 1)
+ %z2 = load i32, ptr @d
+ ret void
+}
+
+; CHECK: !0 = !{i64 {{-?[0-9][0-9]+}}}
+; CHECK: !1 = !{i64 {{-?[0-9][0-9]+}}}
+; CHECK: !2 = !{i64 5}
|
teresajohnson
approved these changes
May 29, 2026
teresajohnson
left a comment
teresajohnson
left a comment
Contributor
There was a problem hiding this comment.
lgtm with a few minor comments
| ret void | ||
| } | ||
|
|
||
| ; CHECK: !0 = !{i64 {{-?[0-9][0-9]+}}} |
Contributor
There was a problem hiding this comment.
Why not compare against the exact guid values we expect in !0 and !1?
Member
Author
There was a problem hiding this comment.
We expect anything but 0 or 1 - because these are supposed to be freshly recomputed. The module name is test-dependent, and, anyway, will be a large int, so we check that we have 2 or more digits.
Merge activity
|
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.
yingopq
pushed a commit
to yingopq/llvm-project
that referenced
this pull request
Jun 5, 2026
…th CFI (llvm#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
2 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.
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::hasMetadataOtherThanDebugLocto...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.