[ThinLTO] Keep non-prevailing (linkonce|weak)_odr symbols live

[trentxintong]

Summary

If we have a symbol with (linkonce|weak)_odr linkage, we do not want
to dead strip it even it is not prevailing.

IR level (linkonce|weak)_odr symbol can become non-prevailing when we mix
ELF objects and IR objects where the (linkonce|weak)_odr symbol in the ELF
object is prevailing and the ones in the IR objects are not. Stripping
them will prevent us from doing optimizations with them.

By not dead stripping them, We will convert these symbols to
available_externally linkage as a result of non-prevailing and eventually
dropping them after inlining.

I modified cache-prevailing.ll to use linkonce linkage as it is
testing whether cache prevailing bit is effective or not, not
we should treat linkonce_odr alive or not

[tejohnson]

This seems reasonable - we should be able to keep ODR non-prevailing defs around to optimize with. I assume this is a missed optimization fix and not a correctness fix (i.e. the build currently succeeds and builds a correct binary)? Question about that below.

lib/Transforms/IPO/FunctionImport.cpp

746 ↗

(On Diff #168325)

Are you fixing another instance of PR36483? If not, then the comment should probably be updated to indicate that this is to fix a missed optimization.

[trentxintong]

lib/Transforms/IPO/FunctionImport.cpp

746 ↗

(On Diff #168325)

Hi Teresa

Thank you for the comment.

We have some linkonceodr symbols that are included in multiple files, one of the files is compiled to ELF object file directly which makes the ones in the IR object non-prevailing.

Originally we crashed because the IR level vtable (linkonce_odr linkage) is not prevailing and WPD crashed. This should be prevented by https://reviews.llvm.org/D52175.

So at this point, this patch is more to improve a missed optimization opportunity than fix a crash. I have the comment "or limit optimization opportunities" in the comment.

[trentxintong]

lib/Transforms/IPO/FunctionImport.cpp

746 ↗

(On Diff #168325)

This also applies to other vague odr symbols we see with template functions, etc.

[tejohnson]

LGTM

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

auto visit = [&](ValueInfo VI) {
     VI = updateValueInfoForIndirectCalls(Index, VI);
     if (!VI)
       return;
     for (auto &S : VI.getSummaryList())
       if (S->isLive())
         return;
 
     // We only keep live symbols that are known to be non-prevailing if any are
-    // available_externally. Those symbols are discarded later in the
-    // EliminateAvailableExternally pass and setting them to not-live breaks
-    // downstreams users of liveness information (PR36483).
+    // available_externally, linkonceodr, weakodr. Those symbols are discarded
+    // later in the EliminateAvailableExternally pass and setting them to
+    // not-live could break downstreams users of liveness information (PR36483)
+    // or limit optimization opportunities.
     if (isPrevailing(VI.getGUID()) == PrevailingType::No) {
-      bool AvailableExternally = false;
+      bool KeepAliveLinkage = false;
       bool Interposable = false;
       for (auto &S : VI.getSummaryList()) {
-        if (S->linkage() == GlobalValue::AvailableExternallyLinkage)
-          AvailableExternally = true;
+        if (S->linkage() == GlobalValue::AvailableExternallyLinkage ||
+            S->linkage() == GlobalValue::WeakODRLinkage ||
+            S->linkage() == GlobalValue::LinkOnceODRLinkage)
+          KeepAliveLinkage = true;
         else if (GlobalValue::isInterposableLinkage(S->linkage()))
           Interposable = true;
       }
 
-      if (!AvailableExternally)
+      if (!KeepAliveLinkage)
         return;
 
       if (Interposable)
-        report_fatal_error("Interposable and available_externally symbol");
+        report_fatal_error(
+          "Interposable and available_externally/linkonce_odr/weak_odr symbol");
     }
 
     for (auto &S : VI.getSummaryList())
       S->setLive(true);
     ++LiveSymbols;
     Worklist.push_back(VI);
   };
 

llvm/trunk/test/LTO/Resolution/X86/cache-prevailing.ll

 ; Tests whether the cache is sensitive to the prevailing bit.
 ; RUN: rm -rf %t.cache
 ; RUN: opt -module-hash -module-summary -o %t.bc %s
 ; RUN: llvm-lto2 run -o %t.o %t.bc -cache-dir %t.cache \
 ; RUN:   -r %t.bc,foo,p -r %t.bc,bar,px
 ; RUN: llvm-lto2 run -o %t.o %t.bc -cache-dir %t.cache \
 ; RUN:   -r %t.bc,foo, -r %t.bc,bar,px
 ; RUN: ls %t.cache | count 2
 
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc19.11.0"
 
-@foo = linkonce_odr constant i32 1, comdat
+@foo = linkonce constant i32 1, comdat
 $foo = comdat any
 
 define i32* @bar() {
   ret i32* @foo
 }

llvm/trunk/test/ThinLTO/X86/deadstrip.ll

 
 ; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t.out -save-temps \
 ; RUN:   -r %t1.bc,_main,plx \
 ; RUN:   -r %t1.bc,_bar,pl \
 ; RUN:   -r %t1.bc,_dead_func,pl \
 ; RUN:   -r %t1.bc,_baz,l \
 ; RUN:   -r %t1.bc,_boo,l \
 ; RUN:   -r %t1.bc,_live_available_externally_func,l \
+; RUN:   -r %t1.bc,_live_linkonce_odr_func,l \
+; RUN:   -r %t1.bc,_live_weak_odr_func,l \
 ; RUN:   -r %t2.bc,_baz,pl \
 ; RUN:   -r %t2.bc,_boo,pl \
 ; RUN:   -r %t2.bc,_dead_func,l \
 ; RUN:   -r %t2.bc,_another_dead_func,pl \
 ; RUN:   -thinlto-threads=1 \
 ; RUN:   -debug-only=function-import 2>&1 | FileCheck %s --check-prefix=DEBUG
 ; RUN: llvm-dis < %t.out.1.3.import.bc | FileCheck %s --check-prefix=LTO2
 ; RUN: llvm-dis < %t.out.2.3.import.bc | FileCheck %s --check-prefix=LTO2-CHECK2
 ; RUN: llvm-nm %t.out.1 | FileCheck %s --check-prefix=CHECK2-NM
 
 ; RUN: llvm-bcanalyzer -dump %t.out.index.bc | FileCheck %s --check-prefix=COMBINED
 ; Live, NotEligibleForImport, dso_local, Internal
 ; COMBINED-DAG: <COMBINED {{.*}} op2=119
 ; Live, dso_local, Internal
 ; COMBINED-DAG: <COMBINED {{.*}} op2=103
+; Live, Local, LinkOnceODR
+; COMBINED-DAG: <COMBINED {{.*}} op2=99
 ; Live, Local, AvailableExternally
 ; COMBINED-DAG: <COMBINED {{.*}} op2=97
 ; Live, Local, External
 ; COMBINED-DAG: <COMBINED {{.*}} op2=96
 ; COMBINED-DAG: <COMBINED {{.*}} op2=96
 ; COMBINED-DAG: <COMBINED {{.*}} op2=96
+; Live, Local, WeakODR
+; COMBINED-DAG: <COMBINED {{.*}} op2=69
 ; Local, (Dead)
 ; COMBINED-DAG: <COMBINED {{.*}} op2=64
 ; COMBINED-DAG: <COMBINED {{.*}} op2=64
 ; COMBINED-DAG: <COMBINED {{.*}} op2=64
 
 ; Dead-stripping on the index allows to internalize these,
 ; and limit the import of @baz thanks to early pruning.
 ; CHECK-NOT: available_externally {{.*}} @baz()

 ; RUN: opt %p/Inputs/deadstrip.ll -o %t3.bc
 ; RUN: llvm-lto2 run %t1.bc %t3.bc -o %t4.out -save-temps \
 ; RUN:   -r %t1.bc,_main,plx \
 ; RUN:   -r %t1.bc,_bar,pl \
 ; RUN:   -r %t1.bc,_dead_func,pl \
 ; RUN:   -r %t1.bc,_baz,l \
 ; RUN:   -r %t1.bc,_boo,l \
 ; RUN:   -r %t1.bc,_live_available_externally_func,l \
+; RUN:   -r %t1.bc,_live_linkonce_odr_func,l \
+; RUN:   -r %t1.bc,_live_weak_odr_func,l \
 ; RUN:   -r %t3.bc,_baz,pl \
 ; RUN:   -r %t3.bc,_boo,pl \
 ; RUN:   -r %t3.bc,_dead_func,l \
 ; RUN:   -r %t3.bc,_another_dead_func,pl
 ; RUN: llvm-dis < %t4.out.1.3.import.bc | FileCheck %s --check-prefix=CHECK-NOTDEAD
 ; RUN: llvm-nm %t4.out.0 | FileCheck %s --check-prefix=CHECK-NM-NOTDEAD
 
 ; We can't internalize @dead_func because of the use in the regular LTO

 
 define void @dead_func() {
     call void @bar()
     call void @baz()
     call void @bar_internal()
     ret void
 }
 
+
+define linkonce_odr void @live_linkonce_odr_func() {
+    ret void
+}
+
+define weak_odr void @live_weak_odr_func() {
+    ret void
+}
+
 define available_externally void @live_available_externally_func() {
     ret void
 }
 
 define void @main() {
     call void @bar()
     call void @bar_internal()
+    call void @live_linkonce_odr_func()
+    call void @live_weak_odr_func()
     call void @live_available_externally_func()
     ret void
 }

llvm/trunk/test/Transforms/FunctionImport/not-prevailing.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary -o %t2.bc %S/Inputs/not-prevailing.ll
 ; RUN: not llvm-lto2 run -o %t3.bc %t1.bc %t2.bc -r %t1.bc,bar,px \
 ; RUN:     -r %t1.bc,foo,x -r %t2.bc,foo,x -save-temps 2>&1 | FileCheck %s
 
-; CHECK: Interposable and available_externally symbol
+; CHECK: Interposable and available_externally/linkonce_odr/weak_odr symbol
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define available_externally i32 @foo() {
   ret i32 1
 }
 
 define i32 @bar() {
   %1 = call i32 @foo()
   ret i32 %1
 }