[ThinLTO] Rename COMDATs for COFF when promoting/renaming COMDAT leader

[tejohnson]

Summary

COFF requires that COMDAT name match that of the leader. When we promote
and rename an internal leader in ThinLTO due to an import, ensure we
subsequently rename the associated COMDAT. Similar to D31963 which did
this during ThinLTO module splitting.

Fixes PR40414.

[inglorion]

lgtm, thanks!

llvm/trunk/include/llvm/Transforms/Utils/FunctionImportUtils.h

class FunctionImportGlobalProcessing {
   /// that any of its functions may be imported into another module
   /// as part of a different backend compilation process.
   bool HasExportedFunctions = false;
 
   /// Set of llvm.*used values, in order to validate that we don't try
   /// to promote any non-renamable values.
   SmallPtrSet<GlobalValue *, 8> Used;
 
+  /// Keep track of any COMDATs that require renaming (because COMDAT
+  /// leader was promoted and renamed). Maps from original COMDAT to one
+  /// with new name.
+  DenseMap<const Comdat *, Comdat *> RenamedComdats;
+
   /// Check if we should promote the given local value to global scope.
   bool shouldPromoteLocalToGlobal(const GlobalValue *SGV);
 
 #ifndef NDEBUG
   /// Check if the given value is a local that can't be renamed (promoted).
   /// Only used in assertion checking, and disabled under NDEBUG since the Used
   /// set will not be populated.
   bool isNonRenamableLocal(const GlobalValue &GV) const;

llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

if (!GV.isDeclaration() && VI && ImportIndex.withGlobalValueDeadStripping()) {
     auto *GVS = SL.empty() ? nullptr : dyn_cast<GlobalVarSummary>(SL[0].get());
     if (GVS && GVS->isReadOnly())
       cast<GlobalVariable>(&GV)->addAttribute("thinlto-internalize");
   }
 
   bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
       ((DoPromote = shouldPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
+    auto Name = GV.getName();
     // Once we change the name or linkage it is difficult to determine
     // again whether we should promote since shouldPromoteLocalToGlobal needs
     // to locate the summary (based on GUID from name and linkage). Therefore,
     // use DoPromote result saved above.
     GV.setName(getName(&GV, DoPromote));
     GV.setLinkage(getLinkage(&GV, DoPromote));
     if (!GV.hasLocalLinkage())
       GV.setVisibility(GlobalValue::HiddenVisibility);
+
+    // If we are renaming a COMDAT leader, ensure that we record the COMDAT
+    // for later renaming as well. This is required for COFF.
+    if (const auto *C = GV.getComdat())
+      if (C->getName() == Name)
+        RenamedComdats.try_emplace(C, M.getOrInsertComdat(GV.getName()));
   } else
     GV.setLinkage(getLinkage(&GV, /* DoPromote */ false));
 
   // Remove functions imported as available externally defs from comdats,
   // as this is a declaration for the linker, and will be dropped eventually.
   // It is illegal for comdats to contain declarations.
   auto *GO = dyn_cast<GlobalObject>(&GV);
   if (GO && GO->isDeclarationForLinker() && GO->hasComdat()) {
     // The IRMover should not have placed any imported declarations in
     // a comdat, so the only declaration that should be in a comdat
     // at this point would be a definition imported as available_externally.
     assert(GO->hasAvailableExternallyLinkage() &&
            "Expected comdat on definition (possibly available external)");
     GO->setComdat(nullptr);
   }
 }
 
 void FunctionImportGlobalProcessing::processGlobalsForThinLTO() {
   for (GlobalVariable &GV : M.globals())
     processGlobalForThinLTO(GV);
   for (Function &SF : M)
     processGlobalForThinLTO(SF);
   for (GlobalAlias &GA : M.aliases())
     processGlobalForThinLTO(GA);
+
+  // Replace any COMDATS that required renaming (because the COMDAT leader was
+  // promoted and renamed).
+  if (!RenamedComdats.empty())
+    for (auto &GO : M.global_objects())
+      if (auto *C = GO.getComdat()) {
+        auto Replacement = RenamedComdats.find(C);
+        if (Replacement != RenamedComdats.end())
+          GO.setComdat(Replacement->second);
+      }
 }
 
 bool FunctionImportGlobalProcessing::run() {
   processGlobalsForThinLTO();
   return false;
 }
 
 bool llvm::renameModuleForThinLTO(Module &M, const ModuleSummaryIndex &Index,
                                   SetVector<GlobalValue *> *GlobalsToImport) {
   FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
   return ThinLTOProcessing.run();
 }

llvm/trunk/test/Transforms/FunctionImport/Inputs/comdat.ll

+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.24215"
+
+define void @main() {
+entry:
+  call i8* @lwt_fun()
+  ret void
+}
+
+declare i8* @lwt_fun()

llvm/trunk/test/Transforms/FunctionImport/comdat.ll

+; Test to ensure that comdat is renamed consistently when comdat leader is
+; promoted and renamed due to an import. Required by COFF.
+
+; REQUIRES: x86-registered-target
+
+; RUN: opt -thinlto-bc -o %t1.bc %s
+; RUN: opt -thinlto-bc -o %t2.bc %S/Inputs/comdat.ll
+; RUN: llvm-lto2 run -save-temps -o %t3 %t1.bc %t2.bc \
+; RUN:          -r %t1.bc,lwt_fun,plx \
+; RUN:          -r %t2.bc,main,plx \
+; RUN:          -r %t2.bc,lwt_fun,
+; RUN: llvm-dis -o - %t3.1.3.import.bc | FileCheck %s
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.24215"
+
+; CHECK: $lwt.llvm.[[HASH:[0-9]+]] = comdat any
+$lwt = comdat any
+
+; CHECK: @lwt_aliasee = private unnamed_addr global {{.*}}, comdat($lwt.llvm.[[HASH]])
+@lwt_aliasee = private unnamed_addr global [1 x i8*] [i8* null], comdat($lwt)
+
+; CHECK: @lwt.llvm.[[HASH]] = hidden unnamed_addr alias
+@lwt = internal unnamed_addr alias [1 x i8*], [1 x i8*]* @lwt_aliasee
+
+; Below function should get imported into other module, resulting in @lwt being
+; promoted and renamed.
+define i8* @lwt_fun() {
+  %1 = getelementptr inbounds [1 x i8*], [1 x i8*]* @lwt, i32 0, i32 0
+  %2 = load i8*, i8** %1
+  ret i8* %2
+}