[Cloning] Copy metadata of global declarations

[ruiling]

Summary

We have modules with metadata on declarations, and out-of-tree passes
use that metadata, and we need to clone those modules. We really expect
such metadata is kept during the clone operation.

[arsenm]

Needs tests

[aprantl]

llvm/unittests/Transforms/Utils/CloningTest.cpp

843 ↗

(On Diff #312448)

Nit: LLVM coding style wants full sentences in comments.
i.e.: // Test global variable declarations.

846 ↗

(On Diff #312448)

It's not obvious to me what the intended difference between GVD and GV is — is it just the Comdat flag? Could we document that?

[ruiling]

llvm/unittests/Transforms/Utils/CloningTest.cpp

843 ↗

(On Diff #312448)

Thanks, will fix it.

846 ↗

(On Diff #312448)

the GVD is just a global variable declaration, which means have no initializer. will document it to make is obvious.

[ruiling]

In D93451#2463049, @arsenm wrote:

Needs tests

Do we need more test besides the unit-test?

[ruiling]

refactor some comments

[ruiling]

ping

[ruiling]

ping, any further comments? @arsenm @aprantl

[arsenm]

Is a lit test possible?

[ruiling]

Now that most features of CloneModule() are tested by the unit-test. Is there any specific reason to prefer lit-test over unit-test?

[arsenm]

In D93451#2481972, @ruiling wrote:

Now that most features of CloneModule() are tested by the unit-test. Is there any specific reason to prefer lit-test over unit-test?

It's easier to read and update IR tests than unit tests

[ruiling]

change to lit-test. @arsenm please check whether you like this.

[thakis]

This caused https://bugs.llvm.org/show_bug.cgi?id=48841

[ruiling]

seems that copying the metadata causing the Cloned module still reference some metadata in source modules. and simplifyExternals(*MergedM); in ThinLTOBitcodeWriter.cpp modifies the metadata in the source module M. which is out-of my expectation. I am not familiar with that part of code, so still need some time to continue investigate. I am not sure @pcc Do you have any idea on this?
Actually it is changing a metadata in module M from:
!32 = !DITemplateValueParameter(type: !33, value: { i64, i64 } { i64 ptrtoint (i32 (%class.i*)* @_ZNK1i5m_fn1Ev to i64), i64 0 })
into
!32 = !DITemplateValueParameter(type: !33, value: { i64, i64 } { i64 ptrtoint (void ()* @_ZNK1i5m_fn1Ev to i64), i64 0 })
I dumped the modules before and after my change, this is the only diff of Module M before writing out.

llvm/lib/Transforms/Utils/CloneModule.cpp

std::unique_ptr<Module> llvm::CloneModule(
   }
 
   // Now that all of the things that global variable initializer can refer to
   // have been created, loop through and copy the global variable referrers
   // over...  We also set the attributes on the global now.
   //
   for (Module::const_global_iterator I = M.global_begin(), E = M.global_end();
        I != E; ++I) {
+    GlobalVariable *GV = cast<GlobalVariable>(VMap[&*I]);
+
+    SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
+    I->getAllMetadata(MDs);
+    for (auto MD : MDs)
+      GV->addMetadata(MD.first,
+                      *MapMetadata(MD.second, VMap, RF_MoveDistinctMDs));
+
     if (I->isDeclaration())
       continue;
 
-    GlobalVariable *GV = cast<GlobalVariable>(VMap[&*I]);
     if (!ShouldCloneDefinition(&*I)) {
       // Skip after setting the correct linkage for an external reference.
       GV->setLinkage(GlobalValue::ExternalLinkage);
       continue;
     }
     if (I->hasInitializer())
       GV->setInitializer(MapValue(I->getInitializer(), VMap));
 
-    SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
-    I->getAllMetadata(MDs);
-    for (auto MD : MDs)
-      GV->addMetadata(MD.first,
-                      *MapMetadata(MD.second, VMap, RF_MoveDistinctMDs));
-
     copyComdat(GV, &*I);
   }
 
   // Similarly, copy over function bodies now...
   //
   for (const Function &I : M) {
     if (I.isDeclaration())
       continue;

llvm/test/Other/copy-metadata-of-declaration.ll

+; RUN: opt -run-twice -verify -S -o - %s | FileCheck %s
+
+; This test is used to check metadata attached to global variable declarations
+; are copied when CloneModule(). This is required by out-of-tree passes.
+
+; CHECK: @g = external addrspace(64) global i32, !spirv.InOut !0
+
+@g = external addrspace(64) global i32, !spirv.InOut !0
+
+!0 = !{i32 1}