Skip to content

Ignore !guid metadata when merging globals or constants#200323

Merged
mtrofin merged 2 commits into
llvm:mainfrom
mtrofin:glob
May 29, 2026
Merged

Ignore !guid metadata when merging globals or constants#200323
mtrofin merged 2 commits into
llvm:mainfrom
mtrofin:glob

Conversation

@mtrofin

@mtrofin mtrofin commented May 29, 2026

Copy link
Copy Markdown
Member

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.

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.
@llvmorg-github-actions

llvmorg-github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Mircea Trofin (mtrofin)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/200323.diff

8 Files Affected:

  • (modified) llvm/include/llvm/IR/GlobalObject.h (+1-1)
  • (modified) llvm/include/llvm/Transforms/Utils/AssignGUID.h (+7)
  • (modified) llvm/lib/CodeGen/GlobalMerge.cpp (+4-1)
  • (modified) llvm/lib/IR/Globals.cpp (+2-2)
  • (modified) llvm/lib/Transforms/IPO/ConstantMerge.cpp (+3-3)
  • (modified) llvm/lib/Transforms/Utils/AssignGUID.cpp (+10-1)
  • (modified) llvm/test/Transforms/ConstantMerge/merge-dbg.ll (+1)
  • (added) llvm/test/Transforms/GlobalMerge/guid.ll (+38)
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 teresajohnson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with a few minor comments

Comment thread llvm/lib/CodeGen/GlobalMerge.cpp
Comment thread llvm/lib/Transforms/IPO/ConstantMerge.cpp
ret void
}

; CHECK: !0 = !{i64 {{-?[0-9][0-9]+}}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not compare against the exact guid values we expect in !0 and !1?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@graphite-app

graphite-app Bot commented May 29, 2026

Copy link
Copy Markdown

Merge activity

  • May 29, 3:30 PM UTC: mtrofin added this pull request to the Graphite merge queue.
  • May 29, 3:32 PM UTC: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Fast-forward merges are not supported for forked repositories. Please create a branch in the target repository in order to merge).

@mtrofin
mtrofin merged commit c4346f2 into llvm:main May 29, 2026
10 checks passed
@mtrofin
mtrofin deleted the glob branch May 29, 2026 15:50
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.
mtrofin added a commit that referenced this pull request Jul 7, 2026
This reverts #201194, thus relanding @orodley's PR #184065 (and
#200323):

> 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

The CFI issues that triggered the original revert are fixed by #201370,
together with the addressing of the TODOs in `LowerTypeTests.cpp` left
in the latter. The [graphite
diff](https://app.graphite.com/github/pr/llvm/llvm-project/201849/Reland-%23184065)
between this change's V1 and V2 shows what's been added:

- the `TODO`s from #201370 are done
- in LowerTypeTests.cpp, passing `!guid` when creating a new declaration
and when converting a definition to a declaration.
- `llvm/test/Transforms/LowerTypeTests/export-icall.ll` tests also the
above def->decl conversion
- removed
`test/Analysis/CtxProfAnalysis/flatten-prethinlink-requires-guid-metadata.ll`
introduced in #194383 (this was between the revert and this PR), as now
the general expectation is that GUID assignment happens appropriately
and all passes use `getGUID`, so there's no reason for `CtxProfAnalysis`
to do something different.

Currently, we reassign GUIDs when CFI promotes internal linkage symbols,
which is counter to the goal of the RFC. This is addressed in PR
#203171. The reason for this split fix can be explained on
`compiler-rt/test/cfi/icall/wrong-signature-mixed-lto.c`. Here, a module
with the exact same source path is compiled twice, under different
conditional compilation, to produce 2 objects. Each object defines an
internal linkage symbol with the same name (this is
`install_trap_loop_detection` from
`compiler-rt/test/cfi/trap_loop_signal_handler.inc` which is
`-include`\-d by both - see how `%clang_cfi` is defined). The ThinLTO
GUID of this symbol will be the same. Its name won't be - because CFI
promotes it and renames it using a hash that is based on the IR Module
content (rather than the source path). During thinlink,
`LTO::addThinLTO`will mark each of the 2 exported symbols as prevailing
in their corresponding modules. But that is done by associating their
GUID to the module. So whichever comes last wins. The other symbol will
be marked available externally and its body DCEd later in backend. But
each module will refer to its copy of `install_trap_loop_detection`, and
so we end up with a linker error.

As mentioned, the fix is in PR #203171, and this relanding PR just
maintains the existing ThinLTO behavior by rewriting the GUIDs. Since we
haven't yet leveraged the GUID mechanics for e.g. simplifying PGO, this
aspect of this change is essentially NFC.

Co-authored-by: Owen Rodley <[email protected]>
gandhi56 pushed a commit that referenced this pull request Jul 9, 2026
This reverts #201194, thus relanding @orodley's PR #184065 (and
#200323):

> 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

The CFI issues that triggered the original revert are fixed by #201370,
together with the addressing of the TODOs in `LowerTypeTests.cpp` left
in the latter. The [graphite
diff](https://app.graphite.com/github/pr/llvm/llvm-project/201849/Reland-%23184065)
between this change's V1 and V2 shows what's been added:

- the `TODO`s from #201370 are done
- in LowerTypeTests.cpp, passing `!guid` when creating a new declaration
and when converting a definition to a declaration.
- `llvm/test/Transforms/LowerTypeTests/export-icall.ll` tests also the
above def->decl conversion
- removed
`test/Analysis/CtxProfAnalysis/flatten-prethinlink-requires-guid-metadata.ll`
introduced in #194383 (this was between the revert and this PR), as now
the general expectation is that GUID assignment happens appropriately
and all passes use `getGUID`, so there's no reason for `CtxProfAnalysis`
to do something different.

Currently, we reassign GUIDs when CFI promotes internal linkage symbols,
which is counter to the goal of the RFC. This is addressed in PR
#203171. The reason for this split fix can be explained on
`compiler-rt/test/cfi/icall/wrong-signature-mixed-lto.c`. Here, a module
with the exact same source path is compiled twice, under different
conditional compilation, to produce 2 objects. Each object defines an
internal linkage symbol with the same name (this is
`install_trap_loop_detection` from
`compiler-rt/test/cfi/trap_loop_signal_handler.inc` which is
`-include`\-d by both - see how `%clang_cfi` is defined). The ThinLTO
GUID of this symbol will be the same. Its name won't be - because CFI
promotes it and renames it using a hash that is based on the IR Module
content (rather than the source path). During thinlink,
`LTO::addThinLTO`will mark each of the 2 exported symbols as prevailing
in their corresponding modules. But that is done by associating their
GUID to the module. So whichever comes last wins. The other symbol will
be marked available externally and its body DCEd later in backend. But
each module will refer to its copy of `install_trap_loop_detection`, and
so we end up with a linker error.

As mentioned, the fix is in PR #203171, and this relanding PR just
maintains the existing ThinLTO behavior by rewriting the GUIDs. Since we
haven't yet leveraged the GUID mechanics for e.g. simplifying PGO, this
aspect of this change is essentially NFC.

Co-authored-by: Owen Rodley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants