[ThinLTO] Respect ClearDSOLocalOnDeclarations for unimported functions

[MaskRay]

Summary

D74751 added ClearDSOLocalOnDeclarations and dropped dso_local for
isDeclarationForLinker GlobalValues. It missed a case for imported
declarations (doImportAsDefinition is false while isPerformingImport is
true). This can lead to a linker error for a default visibility symbol in
ld.lld -shared.

When ClearDSOLocalOnDeclarations is true, we check
isPerformingImport() && !doImportAsDefinition(&GV) along with
GV.isDeclarationForLinker(). The new condition checks an imported declaration.

This patch fixes a LLVMPolly.so link error using a trunk clang -DLLVM_ENABLE_LTO=Thin.

[MaskRay]

Ping:)

13.0.0 will be branched this month and I hope this can be addressed before that. (I could also drop -fno-semantic-interposition for Clang and keep it just for GCC but this issue should be addressed anyway)

[tejohnson]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

129

I don't follow what is happening with and without this change. If we aren't planning to import something as a definition, why do we need to mark it available_externally in the source module? How does this change result in dropping the dso local flag?

From your summary:

i.e. when isPerformingImport, an unimported define dso_local @foo should be treated as an available_externally function, instead of an ExternalLinkage definition.

It should eventually end up as an external declaration. Is that not happening? Or is the available_externally marking just a temporary adjustment to trigger subsequent dso_local clearing?

llvm/test/ThinLTO/X86/import-dsolocal.ll

64

Should this have linkonce_odr linkage?

69

Ditto

[MaskRay]

Address comments

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

129

I got confused. The few cases where getLinkage returns ExternalLinkage means this is an imported declaration (I thought it means an unimported definition).

Returning AvailableExternallyLinkage is not good - we will get declare available_externally and available_externally shouldn't be used on declarations. This is benign because bitcode writer treats available_externally declaration like an external declaration.

I've reverted the part.

[MaskRay]

Test weak/linkonce

[tejohnson]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

128

Is the change of the return linkage type needed for this fix, or is it just the returning of IsImportedDeclaration?

Why not just invoke doImportAsDefinition() in the caller to get this info rather than setting IsImportedDeclaration here? There are also other cases below for other linkage types where we are importing as a declaration but the new var isn't set.

[MaskRay]

use doImportAsDefinition

[tejohnson]

lgtm with test fix noted below.

llvm/test/ThinLTO/X86/import-dsolocal.ll

34

Missing checks for linkonceodr[_aux].

[MaskRay]

improve tests

[MaskRay]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

128

I think invoking doImportAsDefinition is superior. Switched the implementation.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

case GlobalValue::ExternalLinkage:
     // External and linkonce definitions are converted to available_externally
     // definitions upon import, so that they are available for inlining
     // and/or optimization, but are turned into declarations later
     // during the EliminateAvailableExternally pass.
     if (doImportAsDefinition(SGV) && !isa<GlobalAlias>(SGV))
       return GlobalValue::AvailableExternallyLinkage;
     // An imported external declaration stays external.
     return SGV->getLinkage();
 
   case GlobalValue::AvailableExternallyLinkage:
     // An imported available_externally definition converts
     // to external if imported as a declaration.
     if (!doImportAsDefinition(SGV))
       return GlobalValue::ExternalLinkage;
     // An imported available_externally declaration stays that way.
     return SGV->getLinkage();
 
   case GlobalValue::LinkOnceAnyLinkage:
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));
 
   // When ClearDSOLocalOnDeclarations is true, clear dso_local if GV is
   // converted to a declaration, to disable direct access. Don't do this if GV
   // is implicitly dso_local due to a non-default visibility.
-  if (ClearDSOLocalOnDeclarations && GV.isDeclarationForLinker() &&
+  if (ClearDSOLocalOnDeclarations &&
+      (GV.isDeclarationForLinker() ||
+       (isPerformingImport() && !doImportAsDefinition(&GV))) &&
       !GV.isImplicitDSOLocal()) {
     GV.setDSOLocal(false);
   } else if (VI && VI.isDSOLocal(ImportIndex.withDSOLocalPropagation())) {
     // If all summaries are dso_local, symbol gets resolved to a known local
     // definition.
     GV.setDSOLocal(true);
     if (GV.hasDLLImportStorageClass())
       GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);

llvm/test/ThinLTO/X86/import-dsolocal.ll

+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.ll -o %t/a.bc
+; RUN: opt -module-summary %t/b.ll -o %t/b.bc
+
+;; With a small limit, *_aux are either imported declarations (external/linkonce_odr/weak_odr)
+;; or unimported (linkonce/weak). Check we discard dso_local.
+; RUN: llvm-lto2 run %t/a.bc %t/b.bc -o %t1 -save-temps -import-instr-limit=3 \
+; RUN:   -r=%t/a.bc,main,plx -r=%t/a.bc,extern, -r=%t/a.bc,linkonce, -r=%t/a.bc,linkonceodr, -r=%t/a.bc,weak, -r=%t/a.bc,weakodr, \
+; RUN:   -r=%t/b.bc,a,pl -r=%t/b.bc,b,pl -r=%t/b.bc,extern,pl -r=%t/b.bc,extern_aux,pl \
+; RUN:   -r=%t/b.bc,linkonce,pl -r=%t/b.bc,linkonce_aux,pl -r=%t/b.bc,linkonceodr,pl -r=%t/b.bc,linkonceodr_aux,pl \
+; RUN:   -r=%t/b.bc,weak,pl -r=%t/b.bc,weak_aux,pl -r=%t/b.bc,weakodr,pl -r=%t/b.bc,weakodr_aux,pl
+; RUN: llvm-dis %t1.1.3.import.bc -o - | FileCheck %s --check-prefixes=DEST,DEST1
+
+;; With a large limit, *_aux are either imported definitions (external/linkonce_odr/weak_odr)
+;; or unimported (linkonce/weak). Check we discard dso_local as well.
+; RUN: llvm-lto2 run %t/a.bc %t/b.bc -o %t2 -save-temps -import-instr-limit=10 \
+; RUN:   -r=%t/a.bc,main,plx -r=%t/a.bc,extern, -r=%t/a.bc,linkonce, -r=%t/a.bc,linkonceodr, -r=%t/a.bc,weak, -r=%t/a.bc,weakodr, \
+; RUN:   -r=%t/b.bc,a,pl -r=%t/b.bc,b,pl -r=%t/b.bc,extern,pl -r=%t/b.bc,extern_aux,pl \
+; RUN:   -r=%t/b.bc,linkonce,pl -r=%t/b.bc,linkonce_aux,pl -r=%t/b.bc,linkonceodr,pl -r=%t/b.bc,linkonceodr_aux,pl \
+; RUN:   -r=%t/b.bc,weak,pl -r=%t/b.bc,weak_aux,pl -r=%t/b.bc,weakodr,pl -r=%t/b.bc,weakodr_aux,pl
+; RUN: llvm-dis %t2.1.3.import.bc -o - | FileCheck %s --check-prefixes=DEST,DEST2
+
+; DEST:      @a = available_externally global i32 42, align 4
+; DEST-NEXT: @b = external global i32*, align 8
+; DEST:      declare void @linkonce()
+; DEST:      declare void @weak()
+; DEST:      define dso_local i32 @main()
+; DEST:      define available_externally void @extern()
+
+; DEST1:     declare i32 @extern_aux(i32*, i32**)
+; DEST1:     declare i32 @linkonceodr_aux(i32*, i32**)
+; DEST2:     define available_externally i32 @extern_aux(i32* %a, i32** %b)
+; DEST2:     define available_externally i32 @linkonceodr_aux(i32* %a, i32** %b)
+
+; DEST:      define available_externally void @weakodr()
+
+; DEST1:     declare i32 @weakodr_aux(i32*, i32**)
+; DEST2:     define available_externally i32 @weakodr_aux(i32* %a, i32** %b)
+
+;--- a.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @extern()
+declare void @linkonce()
+declare void @linkonceodr()
+declare void @weak()
+declare void @weakodr()
+
+define i32 @main() {
+  call void @extern()
+  call void @linkonce()
+  call void @linkonceodr()
+  call void @weak()
+  call void @weakodr()
+  ret i32 0
+}
+
+;--- b.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@a = dso_local global i32 42, align 4
+@b = dso_local global i32* @a, align 8
+
+define dso_local void @extern() {
+  call i32 @extern_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define dso_local i32 @extern_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}
+
+define linkonce dso_local void @linkonce() {
+  call i32 @linkonce_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define linkonce i32 @linkonce_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}
+
+define linkonce_odr dso_local void @linkonceodr() {
+  call i32 @linkonceodr_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define linkonce_odr i32 @linkonceodr_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}
+
+define weak dso_local void @weak() {
+  call i32 @weak_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define weak i32 @weak_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}
+
+define weak_odr dso_local void @weakodr() {
+  call i32 @weakodr_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define weak_odr i32 @weakodr_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}

llvm/test/ThinLTO/X86/index-const-prop-linkage.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/index-const-prop-linkage.ll -o %t2.bc
 ; RUN: llvm-lto2 run -save-temps %t2.bc -r=%t2.bc,foo,pl -r=%t2.bc,g1,pl -r=%t2.bc,g2,pl -r=%t2.bc,g3, \
 ; RUN:                           %t1.bc -r=%t1.bc,foo, -r=%t1.bc,main,plx -r=%t1.bc,g2,  -o %t3
 ; RUN: llvm-dis %t3.2.3.import.bc -o - | FileCheck %s
 
 ; Check that we never internalize anything with:
 ; - appending linkage
 ; - common linkage
 ; - available_externally linkage
 ; - reference from @llvm.used
 ; CHECK:      @llvm.used = appending global [1 x i32*] [i32* @g2]
-; CHECK-NEXT: @g1 = external dso_local global i32, align 4
+; CHECK-NEXT: @g1 = external global i32, align 4
 ; CHECK-NEXT: @g2 = available_externally global i32 42, align 4
 ; CHECK-NEXT: @g3 = available_externally global i32 42, align 4
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare i32 @foo()
 @g2 = external global i32
 @llvm.used = appending global [1 x i32*] [i32* @g2]
 
 define i32 @main() {
   %v = call i32 @foo()
   ret i32 %v
 }