[GlobalOpt][ConstantMerge] Don't delete a COMDAT key with live associative members#199463
[GlobalOpt][ConstantMerge] Don't delete a COMDAT key with live associative members#199463aleksisch wants to merge 2 commits into
Conversation
A COMDAT key is deleted while an associative member of the same group survives, leaving a dangling comdat reference that later aborts COFF codegen in getComdatGVForCOFF. The CHECK lines reflect today's incorrect output (@key removed); the following fix flips them.
…ative members When LTO internalizes a COMDAT key global to local linkage, GlobalOpt and ConstantMerge could delete it as unused even though associative members of the same group survive. The orphaned members then reference a key symbol that no longer exists, which is aborts codegen in getComdatGVForCOFF: LLVM ERROR: Associative COMDAT symbol '...' does not exist. Closes llvm#199462
|
Hello @aleksisch 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-llvm-transforms Author: Aleksey (aleksisch) ChangesFixes #199462 Full diff: https://github.com/llvm/llvm-project/pull/199463.diff 4 Files Affected:
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
+@key = internal thread_local global i32 0, comdat, align 8
+; CHECK: @assoc = internal constant ptr @initfn{{.*}}comdat($key)
+@assoc = internal constant ptr @initfn, section ".CRT$XDU", comdat($key)
+@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
+@key = internal thread_local global i32 0, comdat, align 8
+; CHECK: @assoc = internal constant ptr @initfn{{.*}}comdat($key)
+@assoc = internal constant ptr @initfn, section ".CRT$XDU", comdat($key)
+@llvm.used = appending global [1 x ptr] [ptr @assoc], section "llvm.metadata"
+
+define internal void @initfn() comdat {
+ ret void
+}
|
Updated message related to AI usage |
aleksisch
changed the title
1 participant
Fixes #199462
The contents of this pull request were substantially written using claude-code. I've reviewed to the best of my ability, hope this helps.