[llvm] Fix comdat removal (PR #199463)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Aleksey (aleksisch)

<details>
<summary>Changes</summary>

Fixes #<!-- -->199462

---
Full diff: https://github.com/llvm/llvm-project/pull/199463.diff


4 Files Affected:

- (modified) llvm/lib/Transforms/IPO/ConstantMerge.cpp (+8-3) 
- (modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+16-4) 
- (added) llvm/test/Transforms/ConstantMerge/comdat-key-associative.ll (+22) 
- (added) llvm/test/Transforms/GlobalOpt/comdat-key-associative.ll (+23) 


``````````diff
diff --git a/llvm/lib/Transforms/IPO/ConstantMerge.cpp b/llvm/lib/Transforms/IPO/ConstantMerge.cpp
index 06e684dbe5a93..6be3e05f8a3d3 100644
--- a/llvm/lib/Transforms/IPO/ConstantMerge.cpp
+++ b/llvm/lib/Transforms/IPO/ConstantMerge.cpp
@@ -160,9 +160,14 @@ static bool mergeConstants(Module &M) {
       // If this GV is dead, remove it.
       GV.removeDeadConstantUsers();
       if (GV.use_empty() && GV.hasLocalLinkage()) {
-        GV.eraseFromParent();
-        ++ChangesMade;
-        continue;
+        // Don't remove a comdat key; it would strand the group's associative
+        // members. Comdat-aware DCE handles dead keys.
+        const Comdat *C = GV.getComdat();
+        if (!C || C->getName() != GV.getName()) {
+          GV.eraseFromParent();
+          ++ChangesMade;
+          continue;
+        }
       }
 
       if (isUnmergeableGlobal(&GV, UsedGlobals))
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 75d0f27169822..4fe98700e0ca0 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1338,8 +1338,11 @@ deleteIfDead(GlobalValue &GV,
   if (!GV.isDiscardableIfUnused() && !GV.isDeclaration())
     return false;
 
+  // Don't delete the comdat key while the group must be kept: it would strand
+  // the group's associative members. (A local non-key member is removable.)
   if (const Comdat *C = GV.getComdat())
-    if (!GV.hasLocalLinkage() && NotDiscardableComdats.count(C))
+    if (NotDiscardableComdats.count(C) &&
+        (!GV.hasLocalLinkage() || GV.getName() == C->getName()))
       return false;
 
   bool Dead;
@@ -1648,7 +1651,8 @@ static bool
 processGlobal(GlobalValue &GV,
               function_ref<TargetTransformInfo &(Function &)> GetTTI,
               function_ref<TargetLibraryInfo &(Function &)> GetTLI,
-              function_ref<DominatorTree &(Function &)> LookupDomTree) {
+              function_ref<DominatorTree &(Function &)> LookupDomTree,
+              const SmallPtrSetImpl<const Comdat *> &NotDiscardableComdats) {
   if (GV.getName().starts_with("llvm."))
     return false;
 
@@ -1679,6 +1683,12 @@ processGlobal(GlobalValue &GV,
   if (GVar->isConstant() || !GVar->hasInitializer())
     return Changed;
 
+  // Don't let processInternalGlobal delete/replace a comdat key while the group
+  // must be kept; that would strand the group's associative members.
+  if (const Comdat *C = GVar->getComdat())
+    if (NotDiscardableComdats.count(C) && GVar->getName() == C->getName())
+      return Changed;
+
   return processInternalGlobal(GVar, GS, GetTTI, GetTLI, LookupDomTree) ||
          Changed;
 }
@@ -1978,7 +1988,8 @@ OptimizeFunctions(Module &M,
       }
     }
 
-    Changed |= processGlobal(F, GetTTI, GetTLI, LookupDomTree);
+    Changed |=
+        processGlobal(F, GetTTI, GetTLI, LookupDomTree, NotDiscardableComdats);
 
     if (!F.hasLocalLinkage())
       continue;
@@ -2077,7 +2088,8 @@ OptimizeGlobalVars(Module &M,
       continue;
     }
 
-    Changed |= processGlobal(GV, GetTTI, GetTLI, LookupDomTree);
+    Changed |= processGlobal(GV, GetTTI, GetTLI, LookupDomTree,
+                             NotDiscardableComdats);
   }
   return Changed;
 }
diff --git a/llvm/test/Transforms/ConstantMerge/comdat-key-associative.ll b/llvm/test/Transforms/ConstantMerge/comdat-key-associative.ll
new file mode 100644
index 0000000000000..5f157798eef98
--- /dev/null
+++ b/llvm/test/Transforms/ConstantMerge/comdat-key-associative.ll
@@ -0,0 +1,22 @@
+; RUN: opt -passes=constmerge -S < %s | FileCheck %s
+
+; @key is the key of a comdat group whose associative member @assoc (pinned by
+; llvm.used) survives. Even though @key is locally unused, constmerge's
+; dead-global cleanup must keep it: deleting the key would leave @assoc
+; referencing a comdat whose key symbol no longer exists, which is illegal for
+; COFF and aborts codegen in getComdatGVForCOFF.
+
+target triple = "x86_64-pc-windows-msvc"
+
+$key = comdat any
+$initfn = comdat any
+
+; CHECK: @key = internal thread_local global i32 0, comdat
+ at key = internal thread_local global i32 0, comdat, align 8
+; CHECK: @assoc = internal constant ptr @initfn{{.*}}comdat($key)
+ at assoc = internal constant ptr @initfn, section ".CRT$XDU", comdat($key)
+ at llvm.used = appending global [1 x ptr] [ptr @assoc], section "llvm.metadata"
+
+define internal void @initfn() comdat {
+  ret void
+}
diff --git a/llvm/test/Transforms/GlobalOpt/comdat-key-associative.ll b/llvm/test/Transforms/GlobalOpt/comdat-key-associative.ll
new file mode 100644
index 0000000000000..046868db2a669
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/comdat-key-associative.ll
@@ -0,0 +1,23 @@
+; RUN: opt -passes=globalopt -S < %s | FileCheck %s
+
+; @key is the key of a comdat group whose associative member @assoc (pinned by
+; llvm.used) survives. Even though @key is locally unused, globalopt must keep
+; it: deleting the key would leave @assoc referencing a comdat whose key symbol
+; no longer exists, which is illegal for COFF and aborts codegen in
+; getComdatGVForCOFF.
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+$key = comdat any
+$initfn = comdat any
+
+; CHECK: @key = internal thread_local {{.*}}global i32 0, comdat
+ at key = internal thread_local global i32 0, comdat, align 8
+; CHECK: @assoc = internal constant ptr @initfn{{.*}}comdat($key)
+ at assoc = internal constant ptr @initfn, section ".CRT$XDU", comdat($key)
+ at llvm.used = appending global [1 x ptr] [ptr @assoc], section "llvm.metadata"
+
+define internal void @initfn() comdat {
+  ret void
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/199463