[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

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

846

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

Thanks, will fix it.

846

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/unittests/Transforms/Utils/CloningTest.cpp

void CreateOldModule() {
     CD->setSelectionKind(Comdat::ExactMatch);
 
     auto GV = new GlobalVariable(
         *OldM, Type::getInt32Ty(C), false, GlobalValue::ExternalLinkage,
         ConstantInt::get(Type::getInt32Ty(C), 1), "gv");
     GV->addMetadata(LLVMContext::MD_type, *MDNode::get(C, {}));
     GV->setComdat(CD);
 
+    // This is a global variable declaration with no initializer.
+    auto GVD = new GlobalVariable(*OldM, Type::getInt32Ty(C), false,
+                                  GlobalValue::ExternalLinkage, nullptr, "gvd");
+    GVD->addMetadata(LLVMContext::MD_type, *MDNode::get(C, {}));
+
     DIBuilder DBuilder(*OldM);
     IRBuilder<> IBuilder(C);
 
     auto *FuncType = FunctionType::get(Type::getVoidTy(C), false);
     auto *PersFn = Function::Create(FuncType, GlobalValue::ExternalLinkage,
                                     "persfn", OldM);
     auto *F =
         Function::Create(FuncType, GlobalValue::PrivateLinkage, "f", OldM);
TEST_F(CloneModule, Subprogram) {
   EXPECT_EQ(SP->getName(), "f");
   EXPECT_EQ(SP->getFile()->getFilename(), "filename.c");
   EXPECT_EQ(SP->getLine(), (unsigned)4);
 }
 
 TEST_F(CloneModule, GlobalMetadata) {
   GlobalVariable *NewGV = NewM->getGlobalVariable("gv");
   EXPECT_NE(nullptr, NewGV->getMetadata(LLVMContext::MD_type));
+  // Test global variable declarations.
+  GlobalVariable *NewGVD = NewM->getGlobalVariable("gvd");
+  EXPECT_NE(nullptr, NewGVD->getMetadata(LLVMContext::MD_type));
 }
 
 TEST_F(CloneModule, GlobalDebugInfo) {
   GlobalVariable *NewGV = NewM->getGlobalVariable("gv");
   EXPECT_TRUE(NewGV != nullptr);
 
   // Find debug info expression assigned to global
   SmallVector<DIGlobalVariableExpression *, 1> GVs;