ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed

Summary:
COFF requires that every comdat contain a symbol with the same name as
the comdat. ThinLTOBitcodeWriter renames symbols, which may cause this
requirement to be violated. This change avoids such violations by
renaming comdats if their leaders are renamed. It also keeps comdats
together when splitting modules.

Reviewers: pcc, mehdi_amini, tejohnson

Reviewed By: pcc

Subscribers: rnk, Prazek, llvm-commits

Differential Revision: https://reviews.llvm.org/D31963

llvm-svn: 300019

lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

@@ -69,15 +69,21 @@ std::string getModuleId(Module *M) {
 // Promote each local-linkage entity defined by ExportM and used by ImportM by
 // changing visibility and appending the given ModuleId.
 void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId) {
+  DenseMap<const Comdat *, Comdat *> RenamedComdats;
   for (auto &ExportGV : ExportM.global_values()) {
     if (!ExportGV.hasLocalLinkage())
       continue;
 
-    GlobalValue *ImportGV = ImportM.getNamedValue(ExportGV.getName());
+    auto Name = ExportGV.getName();
+    GlobalValue *ImportGV = ImportM.getNamedValue(Name);
     if (!ImportGV || ImportGV->use_empty())
       continue;
 
-    std::string NewName = (ExportGV.getName() + ModuleId).str();
+    std::string NewName = (Name + ModuleId).str();
+
+    if (const auto *C = ExportGV.getComdat())
+      if (C->getName() == Name)
+        RenamedComdats.try_emplace(C, ExportM.getOrInsertComdat(NewName));
 
     ExportGV.setName(NewName);
     ExportGV.setLinkage(GlobalValue::ExternalLinkage);
@@ -86,6 +92,14 @@ void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId) {
     ImportGV->setName(NewName);
     ImportGV->setVisibility(GlobalValue::HiddenVisibility);
   }
+
+  if (!RenamedComdats.empty())
+    for (auto &GO : ExportM.global_objects())
+      if (auto *C = GO.getComdat()) {
+        auto Replacement = RenamedComdats.find(C);
+        if (Replacement != RenamedComdats.end())
+          GO.setComdat(Replacement->second);
+      }
 }
 
 // Promote all internal (i.e. distinct) type ids used by the module by replacing
@@ -273,8 +287,13 @@ void splitAndWriteThinLTOBitcode(
   // inline all implementations of the virtual function into each call site,
   // rather than using function attributes to perform local optimization.
   std::set<const Function *> EligibleVirtualFns;
+  // If any member of a comdat lives in MergedM, put all members of that
+  // comdat in MergedM to keep the comdat together.
+  DenseSet<const Comdat *> MergedMComdats;
   for (GlobalVariable &GV : M.globals())
-    if (HasTypeMetadata(&GV))
+    if (HasTypeMetadata(&GV)) {
+      if (const auto *C = GV.getComdat())
+        MergedMComdats.insert(C);
       forEachVirtualFunction(GV.getInitializer(), [&](Function *F) {
         auto *RT = dyn_cast<IntegerType>(F->getReturnType());
         if (!RT || RT->getBitWidth() > 64 || F->arg_empty() ||
@@ -288,10 +307,14 @@ void splitAndWriteThinLTOBitcode(
         if (computeFunctionBodyMemoryAccess(*F, AARGetter(*F)) == MAK_ReadNone)
           EligibleVirtualFns.insert(F);
       });
+    }
 
   ValueToValueMapTy VMap;
   std::unique_ptr<Module> MergedM(
       CloneModule(&M, VMap, [&](const GlobalValue *GV) -> bool {
+        if (const auto *C = GV->getComdat())
+          if (MergedMComdats.count(C))
+            return true;
         if (auto *F = dyn_cast<Function>(GV))
           return EligibleVirtualFns.count(F);
         if (auto *GVar = dyn_cast_or_null<GlobalVariable>(GV->getBaseObject()))
@@ -309,11 +332,15 @@ void splitAndWriteThinLTOBitcode(
       F.setComdat(nullptr);
     }
 
-  // Remove all globals with type metadata, as well as aliases pointing to them,
-  // from the thin LTO module.
+  // Remove all globals with type metadata, globals with comdats that live in
+  // MergedM, and aliases pointing to such globals from the thin LTO module.
   filterModule(&M, [&](const GlobalValue *GV) {
     if (auto *GVar = dyn_cast_or_null<GlobalVariable>(GV->getBaseObject()))
-      return !HasTypeMetadata(GVar);
+      if (HasTypeMetadata(GVar))
+        return false;
+    if (const auto *C = GV->getComdat())
+      if (MergedMComdats.count(C))
+        return false;
     return true;
   });
 

test/Transforms/ThinLTOBitcodeWriter/comdat.ll

@@ -0,0 +1,80 @@
+; RUN: opt -thinlto-bc -o %t %s
+; RUN: llvm-modextract -n 0 -o - %t | llvm-dis | FileCheck --check-prefix=THIN %s
+; RUN: llvm-modextract -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=MERGED %s
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.24215"
+
+; Internal comdat leader with type metadata. All comdat members need to live
+; in the merged module, and the comdat needs to be renamed.
+; MERGED: ${{"?lwt[^ ]+}} = comdat any
+$lwt = comdat any
+
+; External comdat leader, type metadata on non-leader. All comdat
+; members need to live in the merged module, internal members need to
+; be renamed.
+; MERGED: $nlwt = comdat any
+$nlwt = comdat any
+
+; Comdat with two members without type metadata. All comdat members live in
+; the ThinLTO module and no renaming needs to take place.
+; THIN: $nt = comdat any
+$nt = comdat any
+
+; MERGED: @lwt_aliasee = private unnamed_addr global
+; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
+@lwt_aliasee = private unnamed_addr global [1 x i8*] [i8* null], comdat($lwt), !type !0
+
+; MERGED: {{@"?lwt_nl[^ ]+}} = hidden unnamed_addr global
+; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
+; THIN: {{@"?lwt_nl[^ ]+}} = external hidden
+@lwt_nl = internal unnamed_addr global i32 0, comdat($lwt)
+
+; MERGED: @nlwt_aliasee = private unnamed_addr global
+; MERGED-SAME: comdat($nlwt)
+@nlwt_aliasee = private unnamed_addr global [1 x i8*] [i8* null], comdat($nlwt), !type !0
+
+; MERGED: @nlwt = unnamed_addr global
+; MERGED-SAME: comdat
+; THIN: @nlwt = external
+@nlwt = unnamed_addr global i32 0, comdat
+
+; THIN: @nt = internal
+; THIN-SAME: comdat
+@nt = internal unnamed_addr global [1 x i8*] [i8* null], comdat
+
+; THIN: @nt_nl = internal
+; THIN-SAME: comdat($nt)
+@nt_nl = internal unnamed_addr global i32 0, comdat($nt)
+
+; MERGED: {{@"?lwt[^ ]+}} = hidden unnamed_addr alias
+; THIN: {{@"?lwt[^ ]+}} = external hidden
+@lwt = internal unnamed_addr alias [1 x i8*], [1 x i8*]* @lwt_aliasee
+
+; MERGED: {{@"?nlwt_nl[^ ]+}} = hidden unnamed_addr alias
+; THIN: {{@"?nlwt_nl[^ ]+}} = external hidden
+@nlwt_nl = internal unnamed_addr alias [1 x i8*], [1 x i8*]* @nlwt_aliasee
+
+; The functions below exist just to make sure the globals are used.
+define i8* @lwt_fun() {
+  %1 = load i32, i32* @lwt_nl
+  %2 = getelementptr inbounds [1 x i8*], [1 x i8*]* @lwt, i32 0, i32 %1
+  %3 = load i8*, i8** %2
+  ret i8* %3
+}
+
+define i8* @nlwt_fun() {
+  %1 = load i32, i32* @nlwt
+  %2 = getelementptr inbounds [1 x i8*], [1 x i8*]* @nlwt_nl, i32 0, i32 %1
+  %3 = load i8*, i8** %2
+  ret i8* %3
+}
+
+define i8* @nt_fun() {
+  %1 = load i32, i32* @nt_nl
+  %2 = getelementptr inbounds [1 x i8*], [1 x i8*]* @nt, i32 0, i32 %1
+  %3 = load i8*, i8** %2
+  ret i8* %3
+}
+
+!0 = !{i64 8, !"?AVA@@"}