[IRLinker] Don't add duplicate named MD node operand to dest module

[wenju-he]

Fix llvm.ident exploding when linking many bitcode files in libclc. This should de-duplicate other named metadata as well, e.g. opencl.spir.version and opencl.ocl.version.

This PR is a re-submit of https://reviews.llvm.org/D20582 with update that only checks MD node pointer for duplication according to review comment in that PR.

[llvmbot]

@llvm/pr-subscribers-lto

Author: Wenju He (wenju-he)

Changes

Fix llvm.ident exploding when linking many bitcode files in libclc. This should de-duplicate other named metadata as well, e.g. opencl.spir.version and opencl.ocl.version.

This PR is a re-submit of https://reviews.llvm.org/D20582 with update that only checks MD node pointer for duplication according to review comment in that PR.

---

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

5 Files Affected:

• (modified) llvm/lib/Linker/IRMover.cpp (+5-2)
• (modified) llvm/test/Linker/dicompositetype-unique.ll (+2-2)
• (modified) llvm/test/Linker/distinct.ll (+1-1)
• (modified) llvm/test/Linker/unique-fwd-decl-order.ll (+1-1)
• (modified) llvm/test/ThinLTO/X86/import-metadata.ll (+4-3)

diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index a466ce5bf0d4c..b3fa9e2bb0368 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -1133,8 +1133,11 @@ void IRLinker::linkNamedMDNodes() {
 
     NamedMDNode *DestNMD = DstM.getOrInsertNamedMetadata(NMD.getName());
     // Add Src elements into Dest node.
-    for (const MDNode *Op : NMD.operands())
-      DestNMD->addOperand(Mapper.mapMDNode(*Op));
+    for (const MDNode *Op : NMD.operands()) {
+      auto *MD = Mapper.mapMDNode(*Op);
+      if (llvm::find(DestNMD->operands(), MD) == DestNMD->op_end())
+        DestNMD->addOperand(MD);
+    }
   }
 }
 
diff --git a/llvm/test/Linker/dicompositetype-unique.ll b/llvm/test/Linker/dicompositetype-unique.ll
index ab1fdaa3616c9..28b2f33001b9e 100644
--- a/llvm/test/Linker/dicompositetype-unique.ll
+++ b/llvm/test/Linker/dicompositetype-unique.ll
@@ -17,8 +17,8 @@
 
 ; Check that the type map will unique two DICompositeTypes.
 
-; CHECK:   !named = !{!0, !1, !2, !3, !0, !1, !2, !3}
-; NOMAP:   !named = !{!0, !1, !2, !3, !0, !4, !5, !6}
+; CHECK:   !named = !{!0, !1, !2, !3}
+; NOMAP:   !named = !{!0, !1, !2, !3, !4, !5, !6}
 !named = !{!0, !1, !2, !3}
 
 ; Check both directions.
diff --git a/llvm/test/Linker/distinct.ll b/llvm/test/Linker/distinct.ll
index f21c51ce8b54f..0863446f9a33e 100644
--- a/llvm/test/Linker/distinct.ll
+++ b/llvm/test/Linker/distinct.ll
@@ -9,7 +9,7 @@
 ; Add an external reference to @global so that it gets linked in.
 @alias = alias i32, ptr @global
 
-; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !0, !1, !2, !9, !10, !11, !12, !13, !14}
+; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14}
 !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8}
 
 ; CHECK:      !0 = !{}
diff --git a/llvm/test/Linker/unique-fwd-decl-order.ll b/llvm/test/Linker/unique-fwd-decl-order.ll
index e1d8c2e5cf92f..2f83714305b53 100644
--- a/llvm/test/Linker/unique-fwd-decl-order.ll
+++ b/llvm/test/Linker/unique-fwd-decl-order.ll
@@ -8,7 +8,7 @@
 ; Note that these two assembly files number the nodes identically, even though
 ; the nodes are in a different order.  This is for the reader's convenience.
 
-; CHECK: !named = !{!0, !0}
+; CHECK: !named = !{!0}
 !named = !{!0}
 
 ; CHECK: !0 = !{!1}
diff --git a/llvm/test/ThinLTO/X86/import-metadata.ll b/llvm/test/ThinLTO/X86/import-metadata.ll
index f938fdd5c93c9..6ee2ba721c2b1 100644
--- a/llvm/test/ThinLTO/X86/import-metadata.ll
+++ b/llvm/test/ThinLTO/X86/import-metadata.ll
@@ -12,11 +12,12 @@
 
 ; CHECK: !llvm.dbg.cu = !{![[#CU1:]], ![[#CU2:]]}
 ;; Note that MD1 comes from the current module. MD2 is from the imported module. 
-;; We are checking if the imported MD2 doesn't end up having a null operand.
-; CHECK: !llvm.md = !{![[#MD1:]], ![[#MD2:]]}
+;; We are checking that MD2 is combined with MD1 and the imported MD2 doesn't
+;; end up having a null operand.
+; CHECK: !llvm.md = !{![[#MD1:]]}
 ; CHECK: ![[#MD3:]] = !{}
 ; CHECK: ![[#CU2]] = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: ![[#FILE2:]], isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug)
-; CHECK: ![[#MD2]] = !{![[#MD3]]}
+; CHECK: ![[#MD1]] = !{![[#MD3]]}
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-scei-ps4"

[arsenm]

Since that patch got stuck we've had this pass to work around it.

[arsenm]

Can you add tests that cover the behavior for these other nodes:

llvm-project/llvm/lib/Target/AMDGPU/AMDGPUUnifyMetadata.cpp

Line 26 in 60a18d6

const char SpirVer[] = "opencl.spir.version";

I'm not sure if we should opt in for specific named metadata

[bader]

Since that patch got stuck we've had this pass to work around it.

Unfortunately, this problem is not unique to AMDGPU target. Do you think we should make unify-metadata pass target independent and run it for other targets as well (e.g. SPIR-V)?

[arsenm]

Unfortunately, this problem is not unique to AMDGPU target. Do you think we should make unify-metadata pass target independent and run it for other targets as well (e.g. SPIR-V)?

The pass is garbage, the linker should be doing this. A follow up patch should delete it. IMO nobody is ever going to do the suggested work of making uniquability a configurable property for named metadata

[wenju-he]

Since that patch got stuck we've had this pass to work around it.

thanks @arsenm I was looking at history of the AMDGPUUnifyMetadata pass, and then found your patch https://reviews.llvm.org/D20582

[wenju-he]

Can you add tests that cover the behavior for these other nodes:

llvm-project/llvm/lib/Target/AMDGPU/AMDGPUUnifyMetadata.cpp

Line 26 in 60a18d6

const char SpirVer[] = "opencl.spir.version";

I'm not sure if we should opt in for specific named metadata

done in 1a881cc

Differences between this PR's output and AMDGPUUnifyMetadata pass:

• If OpenCL version is different between input modules, linker is keeping all different versions, but the pass keeps the largest version.
• If two nodes in opencl.used.extensions from input modules contain a different string value and share a same string value, linker is treating the nodes as different and both bodes are kept. The pass splits the string value list and keeps a list of unique string values in the final metadata. For instance, MD1, MD2 and MD3 in test llvm/test/Linker/named-metadata-opencl.ll will have following output for the pass:

; CHECK: ![[#MD1]] = !{!"cl_images"}
; CHECK: ![[#MD2]] = !{!"cl_doubles"}
; CHECK: ![[#MD3]] = !{!"cl_khr_fp16"}