Don't add repeats of llvm.ident list when linking

[arsenm]

Summary

Fixes spam in the common case when linking together
many modules produced by the same compiler version.

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[jvesely]

Is there a reason not to dedup all metadata nodes?

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[dexonsmith]

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

[arsenm]

In D20582#1407184, @dexonsmith wrote:

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

Well the linker API only exposes linking one module into one other at at time, so I don't know. Currently we have the AMDGPUUnifyMetadata pass as a workaround which cleans these up in a pass over the fully linked module, so that avoids revisiting for each module but it would make more sense if the linker dealt with this.

[dexonsmith]

In D20582#1407226, @arsenm wrote:

In D20582#1407184, @dexonsmith wrote:

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

Well the linker API only exposes linking one module into one other at at time, so I don't know. Currently we have the AMDGPUUnifyMetadata pass as a workaround which cleans these up in a pass over the fully linked module, so that avoids revisiting for each module but it would make more sense if the linker dealt with this.

I wonder whether it would be generally useful to support SetVectors in named metadata nodes. I'm not sure what the textual IR syntax would be, but I believe debug info would use it too.

lib/Linker/IRMover.cpp

1001–1005

Do you need to reach inside to the strings? Why not just compare the nodes themselves?

[arsenm]

In D20582#1407227, @dexonsmith wrote:

In D20582#1407226, @arsenm wrote:

In D20582#1407184, @dexonsmith wrote:

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

Well the linker API only exposes linking one module into one other at at time, so I don't know. Currently we have the AMDGPUUnifyMetadata pass as a workaround which cleans these up in a pass over the fully linked module, so that avoids revisiting for each module but it would make more sense if the linker dealt with this.

I wonder whether it would be generally useful to support SetVectors in named metadata nodes. I'm not sure what the textual IR syntax would be, but I believe debug info would use it too.

Something more general would be better. The custom pass currently handles 6 different items

[dexonsmith]

Sadly I missed your last comment and this has languished :/.

If you are still interested in pursuing this, I suggest adding setvector semantics to NamedMetadata as an option, and then this patch is just migrating llvm.ident to it. A couple of comptions for syntax include:

!name{} = !{!0}

!name{uniqued} = !{!0}

!name! = !{!0}

!name& = !{!0}

!name &= !{!0}

!name |= !{!0}

Please CC me on the RFC to llvm-dev.

lib/Linker/IRMover.cpp

 //===- lib/Linker/IRMover.cpp ---------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Linker/IRMover.h"
 #include "LinkDiagnosticInfo.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GVMaterializer.h"
 #include "llvm/IR/TypeFinder.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 using namespace llvm;

 /// Insert all of the named MDNodes in Src into the Dest module.
 void IRLinker::linkNamedMDNodes() {
   const NamedMDNode *SrcModFlags = SrcM->getModuleFlagsMetadata();
   for (const NamedMDNode &NMD : SrcM->named_metadata()) {
     // Don't link module flags here. Do them separately.
     if (&NMD == SrcModFlags)
       continue;
     NamedMDNode *DestNMD = DstM.getOrInsertNamedMetadata(NMD.getName());
+
+    if (NMD.getName() == "llvm.ident") {
+      StringSet<> UniqueIdents;
+
+      // Unique the compiler IDs linked together.
+      for (const MDNode *DestID : DestNMD->operands()) {
+        if (const MDString *VerStr
+            = cast_or_null<MDString>(DestID->getOperand(0)))
+          UniqueIdents.insert(VerStr->getString());
+      }
+
+      for (const MDNode *Op : NMD.operands()) {
+        if (const MDString *VerStr = cast_or_null<MDString>(Op->getOperand(0))) {
+          if (UniqueIdents.insert(VerStr->getString()).second)
+            DestNMD->addOperand(Mapper.mapMDNode(*Op));
+        }
+      }
+
+      continue;
+    }
+
     // Add Src elements into Dest node.
     for (const MDNode *Op : NMD.operands())
       DestNMD->addOperand(Mapper.mapMDNode(*Op));
   }
 }
 
 /// Merge the linker flags in Src into the Dest module.
 bool IRLinker::linkModuleFlagsMetadata() {

test/Linker/ident.ll

-; RUN: llvm-link %S/Inputs/ident.a.ll %S/Inputs/ident.b.ll -S | FileCheck %s
+; RUN: llvm-link %S/Inputs/ident.a.ll %S/Inputs/ident.b.ll %S/Inputs/ident.c.ll -S | FileCheck %s
 
-; Verify that multiple input llvm.ident metadata are linked together.
+; Verify that multiple input llvm.ident metadata are linked together, but uniqued.
 
 ; CHECK-DAG: !llvm.ident = !{!0, !1, !2}
 ; CHECK-DAG: "Compiler V1"
 ; CHECK-DAG: "Compiler V2"
 ; CHECK-DAG: "Compiler V3"