[Linker] Do not keep a private member of a non-prevailing comdat group #69143

igorkudrin · 2023-10-16T05:23:17Z

IRMover links in referenced private global values unconditionally, see IRLinker::shouldLink(). If they are part of a non-prevailing comdat, this leads to duplication of the values. Full and Thin LTO avoid duplication by changing the linkage of members of non-prevailing comdat groups to available_externally, which was implemented in https://reviews.llvm.org/D34803 and https://reviews.llvm.org/D135427. This patch does the same for Linker, but limits the effect only to private members without aliases to minimize interference.

Motivation example:

> cat foo.h
inline int foo(int a) { return a + 1; }
> cat bar.cpp
#include "foo.h"
int bar(int a) { return foo(a + 1); }
> cat main.cpp
#include "foo.h"
int bar(int a);
int main(int argc, const char* argv[]) { return bar(argc) + foo(argc); }
> clang++ -c -flto -fprofile-instr-generate main.cpp -o main.o
> clang++ -c -flto -fprofile-instr-generate bar.cpp -o bar.o
> clang++ -fuse-ld=lld -fprofile-instr-generate main.o bar.o -o test1
> ./test1
> llvm-profdata merge --text default.profraw -o -
_Z3fooi
# Counter Values:
2
> llvm-link main.o bar.o -o combined.o
> clang++ -fuse-ld=lld -fprofile-instr-generate combined.o -o test2
> ./test2
> llvm-profdata merge --text default.profraw -o -
_Z3fooi
# Counter Values:
4

llvmbot · 2023-10-16T05:24:25Z

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-lto

Author: Igor Kudrin (igorkudrin)

Changes

IRMover links in referenced private global values unconditionally, see IRLinker::shouldLink(). If they are part of a non-prevailing comdat, this leads to duplication of the values. Full and Thin LTO avoid duplication by changing the linkage of members of non-prevailing comdat groups to available_externally, which was implemented in https://reviews.llvm.org/D34803 and https://reviews.llvm.org/D135427. This patch does the same for Linker, but limits the effect only to private members without aliases to minimize interference.

Motivation example:

&gt; cat foo.h
inline int foo(int a) { return a + 1; }
int bar(int a);
&gt; cat bar.cpp
#include "foo.h"
int bar(int a) { return foo(a + 1); }
&gt; cat main.cpp
#include "foo.h"
int main(int argc, const char* argv[]) { return bar(argc) + foo(argc); }
&gt; clang -c -flto -fprofile-instr-generate main.cpp -o main.o
&gt; clang -c -flto -fprofile-instr-generate bar.cpp -o bar.o
&gt; clang -fuse-ld=lld -fprofile-instr-generate main.o bar.o -o test1
&gt; ./test1
&gt; llvm-profdata merge --text default.profraw -o -
_Z3fooi
# Counter Values:
2
&gt; llvm-link main.o bar.o -o combined.o
&gt; clang -fuse-ld=lld -fprofile-instr-generate combined.o -o test2
&gt; ./test2
&gt; llvm-profdata merge --text default.profraw -o -
_Z3fooi
# Counter Values:
4

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

2 Files Affected:

(modified) llvm/lib/Linker/LinkModules.cpp (+21)
(modified) llvm/test/Linker/comdat-nonprevailing-decl.ll (+29)

diff --git a/llvm/lib/Linker/LinkModules.cpp b/llvm/lib/Linker/LinkModules.cpp
index 2f5fac4951f29e0..4fe1f1a0f51833f 100644
--- a/llvm/lib/Linker/LinkModules.cpp
+++ b/llvm/lib/Linker/LinkModules.cpp
@@ -462,6 +462,7 @@ void ModuleLinker::dropReplacedComdat(
 bool ModuleLinker::run() {
   Module &DstM = Mover.getModule();
   DenseSet<const Comdat *> ReplacedDstComdats;
+  DenseSet<const Comdat *> NonPrevailingComdats;
 
   for (const auto &SMEC : SrcM->getComdatSymbolTable()) {
     const Comdat &C = SMEC.getValue();
@@ -473,6 +474,9 @@ bool ModuleLinker::run() {
       return true;
     ComdatsChosen[&C] = std::make_pair(SK, From);
 
+    if (From == LinkFrom::Dst)
+      NonPrevailingComdats.insert(&C);
+
     if (From != LinkFrom::Src)
       continue;
 
@@ -497,6 +501,23 @@ bool ModuleLinker::run() {
   for (Function &GV : llvm::make_early_inc_range(DstM))
     dropReplacedComdat(GV, ReplacedDstComdats);
 
+  if (!NonPrevailingComdats.empty()) {
+    DenseSet<GlobalObject *> AliasedGlobals;
+    for (auto &GA : SrcM->aliases())
+      if (GlobalObject *GO = GA.getAliaseeObject(); GO && GO->getComdat())
+        AliasedGlobals.insert(GO);
+    for (const Comdat *C : NonPrevailingComdats) {
+      SmallVector<GlobalObject *> ToUpdate;
+      for (GlobalObject *GO : C->getUsers())
+        if (GO->hasPrivateLinkage() && !AliasedGlobals.contains(GO))
+          ToUpdate.push_back(GO);
+      for (GlobalObject *GO : ToUpdate) {
+        GO->setLinkage(GlobalValue::AvailableExternallyLinkage);
+        GO->setComdat(nullptr);
+      }
+    }
+  }
+
   for (GlobalVariable &GV : SrcM->globals())
     if (GV.hasLinkOnceLinkage())
       if (const Comdat *SC = GV.getComdat())
diff --git a/llvm/test/Linker/comdat-nonprevailing-decl.ll b/llvm/test/Linker/comdat-nonprevailing-decl.ll
index d3dfec8fb2bc728..30dd0e0abf8c56c 100644
--- a/llvm/test/Linker/comdat-nonprevailing-decl.ll
+++ b/llvm/test/Linker/comdat-nonprevailing-decl.ll
@@ -1,5 +1,6 @@
 ; RUN: rm -rf %t && split-file %s %t
 ; RUN: llvm-link -S %t/1.ll %t/1-aux.ll -o - | FileCheck %s
+; RUN: llvm-link -S %t/2.ll %t/2-aux.ll -o - | FileCheck %s --check-prefix=CHECK2
 
 ;--- 1.ll
 $c = comdat any
@@ -23,3 +24,31 @@ define ptr @f3() {
   ret ptr @v3
 }
 
+;--- 2.ll
+;; Check that a private global variable from a non-prevailing comdat group is
+;; converted into 'available_externally' and excluded from the comdat group.
+
+; CHECK2: $__profc_foo = comdat any
+; CHECK2: @llvm.compiler.used = appending global [2 x ptr] [ptr @__profd_foo.[[SUFFIX:[0-9]+]], ptr @__profd_foo]
+; CHECK2: @__profd_foo.[[SUFFIX]] = private global ptr @__profc_foo, comdat($__profc_foo)
+; CHECK2: @__profc_foo = linkonce_odr global i64 1, comdat
+; CHECK2: @__profd_foo = available_externally dso_local global ptr @__profc_foo{{$}}
+
+$__profc_foo = comdat any
+@__profc_foo = linkonce_odr global i64 1, comdat
+@__profd_foo = private global ptr @__profc_foo, comdat($__profc_foo)
+@llvm.compiler.used = appending global [1 x ptr] [ ptr @__profd_foo ]
+
+define ptr @bar() {
+  ret ptr @__profc_foo
+}
+
+;--- 2-aux.ll
+$__profc_foo = comdat any
+@__profc_foo = linkonce_odr global i64 1, comdat
+@__profd_foo = private global ptr @__profc_foo, comdat($__profc_foo)
+@llvm.compiler.used = appending global [1 x ptr] [ ptr @__profd_foo ]
+
+define ptr @baz() {
+  ret ptr @__profc_foo
+}

igorkudrin · 2023-10-23T21:33:48Z

Ping

rnk

The premerge test failure looks unrelated. I think this is good, with a small modification.

rnk · 2023-10-23T22:55:21Z

llvm/lib/Linker/LinkModules.cpp

+    for (const Comdat *C : NonPrevailingComdats) {
+      SmallVector<GlobalObject *> ToUpdate;
+      for (GlobalObject *GO : C->getUsers())
+        if (GO->hasPrivateLinkage() && !AliasedGlobals.contains(GO))


I think it's more correct to use hasLocalLinkage instead of hasPrivateLinkage. Looking at the IRMover call sites, that's what's used there.

This change would break the Linker/comdat-refer-to-discarded.ll test. Originally, it was Linker/comdat13.ll in baa3bf8, which stated that the duplication of internal globals "matches the expectations of COFF linkers`. Since no high-level example was provided, it is now quite difficult, at least for me, to judge whether that behavior is correct or not, so I would prefer not to change it in my patch.

Looking at the test and the history, it is still valid. I think the bug here is that these comdats referring to COFF internal linkage entities are considered NonPrevailingComdats. The proper fix for that is probably in some other part of the code.

I think we should go ahead and land this as is. I'll file an issue about the logic bug I think I see.

for (auto &GA : SrcM->aliases()) is not tested. Add some tests?

for (auto &GA : SrcM->aliases()) is not tested. Add some tests?

This is covered by Linker/comdat-any.ll, case 3.

rnk · 2023-10-26T21:28:18Z

llvm/lib/Linker/LinkModules.cpp

+    for (const Comdat *C : NonPrevailingComdats) {
+      SmallVector<GlobalObject *> ToUpdate;
+      for (GlobalObject *GO : C->getUsers())
+        if (GO->hasPrivateLinkage() && !AliasedGlobals.contains(GO))


Looking at the test and the history, it is still valid. I think the bug here is that these comdats referring to COFF internal linkage entities are considered NonPrevailingComdats. The proper fix for that is probably in some other part of the code.

rnk · 2023-10-26T21:30:50Z

llvm/lib/Linker/LinkModules.cpp

@@ -473,6 +474,9 @@ bool ModuleLinker::run() {
      return true;
    ComdatsChosen[&C] = std::make_pair(SK, From);

+    if (From == LinkFrom::Dst)


I think for COFF (not for ELF) in the comdat-refer-to-discarded.ll test case we should get a result of LinkFrom::Both here.

If I remember it right, LinkFrom::Both corresponds to comdat nodeduplicate, while comdat-refer-to-discarded.ll uses comdat any. Do you think the interpretation of a comdat selection type should depend on the target platform (COFF vs ELF)?

rnk · 2023-10-26T21:39:23Z

llvm/lib/Linker/LinkModules.cpp

+    for (const Comdat *C : NonPrevailingComdats) {
+      SmallVector<GlobalObject *> ToUpdate;
+      for (GlobalObject *GO : C->getUsers())
+        if (GO->hasPrivateLinkage() && !AliasedGlobals.contains(GO))


I think we should go ahead and land this as is. I'll file an issue about the logic bug I think I see.

rnk

Actually, I was able to think of some practical cases this would break, let me try to construct them.

rnk · 2023-10-26T21:54:06Z

Won't your change do the wrong thing for cases like this? https://gcc.godbolt.org/z/qYfbEY1Ee

static int arr[32];
int *useit() { return &arr[0]; }

Compile with: -O2 -emit-llvm --target=x86_64-windows-msvc -fsanitize=address

Relevant IR output:

$arr = comdat nodeduplicate
@arr = internal global { [32 x i32], [32 x i8] } zeroinitializer, comdat, align 32
...
@__asan_global_arr = private global { i64, i64, i64, i64, i64, i64, i64, i64 } { i64 ptrtoint (ptr @arr to i64), i64 128, i64 160, i64 ptrtoint (ptr @___asan_gen_.1 to i64), i64 ptrtoint (ptr @___asan_gen_ to i64), i64 0, i64 0, i64 -1 }, section ".ASAN$GL", comdat($arr), align 64

Alright, so I've convinced myself that this works today because ASan uses the nodeduplicate type here, which gives us the "both" selection kind, and we get the desired semantics.

MaskRay · 2023-10-27T01:58:00Z

> cat foo.h
inline int foo(int a) { return a + 1; }
int bar(int a);

Remove int bar(int a);

igorkudrin · 2023-10-28T00:04:55Z

Remove int bar(int a);

Why? main() calls bar(), so this declaration is required.

MaskRay · 2023-10-28T00:13:24Z

int bar(int a);

int bar(int a); is unrelated to foo.h. The declaration can be moved to main.cpp

edit: I think this is fine.

`IRMover` links in referenced private global values unconditionally, see `IRLinker::shouldLink()`. If they are part of a non-prevailing comdat, this leads to duplication of the values. Full and Thin LTO avoid duplication by changing the linkage of members of non-prevailing comdat groups to `available_externally`, which was implemented in https://reviews.llvm.org/D34803 and https://reviews.llvm.org/D135427. This patch does the same for `Linker`, but limits the effect only to private members without aliases to minimize interference. Motivation example: ``` > cat foo.h inline int foo(int a) { return a + 1; } > cat bar.cpp #include "foo.h" int bar(int a) { return foo(a + 1); } > cat main.cpp #include "foo.h" int bar(int a); int main(int argc, const char* argv[]) { return bar(argc) + foo(argc); } > clang++ -c -flto -fprofile-instr-generate main.cpp -o main.o > clang++ -c -flto -fprofile-instr-generate bar.cpp -o bar.o > clang++ -fuse-ld=lld -fprofile-instr-generate main.o bar.o -o test1 > ./test1 > llvm-profdata merge --text default.profraw -o - _Z3fooi # Counter Values: 2 > llvm-link main.o bar.o -o combined.o > clang++ -fuse-ld=lld -fprofile-instr-generate combined.o -o test2 > ./test2 > llvm-profdata merge --text default.profraw -o - _Z3fooi # Counter Values: 4 ```

igorkudrin requested review from rnk, MaskRay, teresajohnson and dwblaikie October 16, 2023 05:23

llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label Oct 16, 2023

igorkudrin added tools:llvm-link PGO Profile Guided Optimizations labels Oct 16, 2023

igorkudrin force-pushed the linker-private-in-nonprevailing-comdat branch 2 times, most recently from b8a12db to 29eca76 Compare October 23, 2023 21:33

rnk approved these changes Oct 23, 2023

View reviewed changes

rnk approved these changes Oct 26, 2023

View reviewed changes

rnk self-requested a review October 26, 2023 21:41

rnk reviewed Oct 26, 2023

View reviewed changes

MaskRay approved these changes Oct 27, 2023

View reviewed changes

igorkudrin force-pushed the linker-private-in-nonprevailing-comdat branch from 29eca76 to 8f51816 Compare October 28, 2023 00:52

igorkudrin merged commit b1554fe into llvm:main Oct 28, 2023

igorkudrin deleted the linker-private-in-nonprevailing-comdat branch November 21, 2023 03:41

igorkudrin mentioned this pull request Aug 24, 2024

[libLTO, IPGO] Slowdown after b1554fe #105836

Open

[Linker] Do not keep a private member of a non-prevailing comdat group #69143

[Linker] Do not keep a private member of a non-prevailing comdat group #69143

Uh oh!

Conversation

igorkudrin commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorkudrin commented Oct 23, 2023

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorkudrin Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

rnk commented Oct 26, 2023

Uh oh!

MaskRay commented Oct 27, 2023

Uh oh!

igorkudrin commented Oct 28, 2023

Uh oh!

MaskRay commented Oct 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

igorkudrin commented Oct 16, 2023 •

edited

Loading

llvmbot commented Oct 16, 2023 •

edited

Loading

igorkudrin Oct 27, 2023 •

edited

Loading

MaskRay commented Oct 28, 2023 •

edited

Loading