[ThinLTO] Fix internalization decisions for weak/linkonce ODR

This fixes a runtime error that occurred due to incorrect
internalization of linkonce_odr functions where function pointer
equality was broken. This was hit because the prevailing copy was in a
native object, so the IR copies were not exported, and the existing code
internalized all of the IR copies. It could be fixed by guarding this
internalization on whether the defs are (local_)unnamed_addr, meaning
that their address is not significant (which we have in the summary
currently for linkonce_odr via the CanAutoHide flag). Or we can
propagate reference attributes as we do when determining whether a
global variable is read or write-only (reference edges are annotated
with whether they are read-only, write-only, or neither, and taking the
address of a function would result in a reference edge to the function
that is not read or write-only).

However, this exposed a larger issue with the internalization handling.
Looking at test cases, it appears the intent is to internalize when
there is a single definition of a linkonce/weak ODR symbol (that isn't
exported). This makes sense in the case of functions, because the
inliner can apply its last call to static heuristic when appropriate. In
the case where there is no prevailing copy in IR, internalizing all of
the IR copies of a linkonce_odr, even if legal, just increases binary
size. In that case it is better to fall back to the normal handling of
converting all non-prevailing copies to available_externally so that
they are eliminated after inlining.

In the case of variables, the existing code was attempting to
internalize the non-exported linkonce/weak ODR variables if they were
read or write-only. While this is legal (we propagate reference
attributes to determine this information), we don't even need to
internalize these here as there is later separate handling that
internalizes read and write-only variables when we process the module at
the start of the ThinLTO backend (processGlobalForThinLTO). Instead, we
can also internalize any non-exported variable when there is only one
(IR) definition, which is prevailing. And in that case, we don't need to
require that it is read or write-only, since we are guaranteed that all
uses must use that single definition.

In the new LTO API, if there are multiple defs of a linkonce or weak ODR
it will be marked exported, but it isn't clear that this will always be
true for the legacy LTO API. Therefore, require that there is only a
single (non-local) def, and that it is prevailing.

The test cases changes are both to reflect the change in the handling of
linkonce_odr IR copies where the prevailing def is not in IR (the main
correctness bug fix here), and to reflect the more aggressive
internalization of variables when there is only a single def, it is in
IR, and not exported.

I've also added some additional testing via the new LTO API.

Differential Revision: https://reviews.llvm.org/D151965

3 files changed, 59 insertions, 17 deletions

diff --git a/llvm/test/ThinLTO/X86/not-internalized.ll b/llvm/test/ThinLTO/X86/not-internalized.ll
index cf3ae05..5803f05 100644
--- a/llvm/test/ThinLTO/X86/not-internalized.ll
+++ b/llvm/test/ThinLTO/X86/not-internalized.ll
@@ -6,7 +6,7 @@ target triple = "x86_64-unknown-linux-gnu"
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: llvm-lto2 run -save-temps %t.bc -o %t.out \
 ; RUN:    -r=%t.bc,foo,plx \
-; RUN:    -r=%t.bc,bar,lx
+; RUN:    -r=%t.bc,bar,pl
 
 ; Check that we don't internalize `bar` during promotion,
 ; because foo and bar are members of the same comdat
diff --git a/llvm/test/ThinLTO/X86/weak_externals.ll b/llvm/test/ThinLTO/X86/weak_externals.ll
index 02e7888..f206bd8 100644
--- a/llvm/test/ThinLTO/X86/weak_externals.ll
+++ b/llvm/test/ThinLTO/X86/weak_externals.ll
@@ -11,8 +11,13 @@
 
 ; CHECK: @_ZZN9SingletonI1SE11getInstanceEvE8instance = available_externally dso_local global %struct.S zeroinitializer
 ; CHECK: @_ZZN9SingletonI1SE11getInstanceEvE13instance_weak = available_externally dso_local global ptr null, align 8
-; CHECK: define linkonce_odr dso_local dereferenceable(16) ptr @_ZN9SingletonI1SE11getInstanceEv() comdat
-; INTERNALIZE: define internal dereferenceable(16) ptr @_ZN9SingletonI1SE11getInstanceEv()
+
+;; We should not internalize a linkonce_odr function when the IR definition(s)
+;; are not prevailing (prevailing def in native object). This can break function
+;; pointer equality (unless it has an unnamed_addr attribute indicating that the
+;; address is not significant), and also can increase code size.
+; CHECK: define available_externally dso_local dereferenceable(16) ptr @_ZN9SingletonI1SE11getInstanceEv()
+; INTERNALIZE: define available_externally dso_local dereferenceable(16) ptr @_ZN9SingletonI1SE11getInstanceEv()
 
 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"
diff --git a/llvm/test/ThinLTO/X86/weak_resolution.ll b/llvm/test/ThinLTO/X86/weak_resolution.ll
index c7f24e4..1bca0c7 100644
--- a/llvm/test/ThinLTO/X86/weak_resolution.ll
+++ b/llvm/test/ThinLTO/X86/weak_resolution.ll
@@ -1,33 +1,66 @@
-; Do setup work for all below tests: generate bitcode and combined index
+;; Test to ensure we properly resolve weak symbols and internalize them when
+;; appropriate.
+
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/weak_resolution.ll -o %t2.bc
-; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
 
-; Verify that prevailing weak for linker symbol is selected across modules,
-; non-prevailing ODR are not kept when possible, but non-ODR non-prevailing
-; are not affected.
+;; First try this with the legacy LTO API
+; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
+;; Verify that prevailing weak for linker symbol is selected across modules,
+;; non-prevailing ODR are not kept when possible, but non-ODR non-prevailing
+;; are not affected.
 ; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=MOD1
 ; RUN: llvm-lto -thinlto-action=internalize %t.bc -thinlto-index=%t3.bc -exported-symbol=_linkoncefunc -o - | llvm-dis -o - | FileCheck %s --check-prefix=MOD1-INT
 ; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=MOD2
 ; When exported, we always preserve a linkonce
 ; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc -o - --exported-symbol=_linkonceodrfuncInSingleModule | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTED
 
+;; Now try this with the new LTO API
+; RUN: llvm-lto2 run %t.bc %t2.bc -o %t3.out -save-temps \
+; RUN:   -r %t.bc,_linkonceodralias,pl \
+; RUN:   -r %t.bc,_linkoncealias,pl \
+; RUN:   -r %t.bc,_linkonceodrvarInSingleModule,pl \
+; RUN:   -r %t.bc,_weakodrvarInSingleModule,pl \
+; RUN:   -r %t.bc,_linkonceodrfuncwithalias,pl \
+; RUN:   -r %t.bc,_linkoncefuncwithalias,pl \
+; RUN:   -r %t.bc,_linkonceodrfunc,pl \
+; RUN:   -r %t.bc,_linkoncefunc,pl \
+; RUN:   -r %t.bc,_weakodrfunc,pl \
+; RUN:   -r %t.bc,_weakfunc,pl \
+; RUN:   -r %t.bc,_linkonceodrfuncInSingleModule,pl \
+; RUN:   -r %t2.bc,_linkonceodrfuncwithalias,l \
+; RUN:   -r %t2.bc,_linkoncefuncwithalias,l \
+; RUN:   -r %t2.bc,_linkonceodrfunc,l \
+; RUN:   -r %t2.bc,_linkoncefunc,l \
+; RUN:   -r %t2.bc,_weakodrfunc,l \
+; RUN:   -r %t2.bc,_weakfunc,l \
+; RUN:   -r %t2.bc,_linkonceodralias,l \
+; RUN:   -r %t2.bc,_linkoncealias,l
+; RUN: llvm-dis %t3.out.1.2.internalize.bc -o - | FileCheck %s --check-prefix=MOD1-INT
+
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
-; Alias are resolved, but can't be turned into "available_externally"
+;; Alias are resolved, but can't be turned into "available_externally"
 ; MOD1: @linkonceodralias = weak_odr alias void (), ptr @linkonceodrfuncwithalias
 ; MOD2: @linkonceodralias = linkonce_odr alias void (), ptr @linkonceodrfuncwithalias
 @linkonceodralias = linkonce_odr alias void (), ptr @linkonceodrfuncwithalias
 
-; Alias are resolved, but can't be turned into "available_externally"
+;; Alias are resolved, but can't be turned into "available_externally"
 ; MOD1: @linkoncealias = weak alias void (), ptr @linkoncefuncwithalias
 ; MOD2: @linkoncealias = linkonce alias void (), ptr @linkoncefuncwithalias
 @linkoncealias = linkonce alias void (), ptr @linkoncefuncwithalias
 
-; Function with an alias are resolved to weak_odr in prevailing module, but
-; not optimized in non-prevailing module (illegal to have an
-; available_externally aliasee).
+;; Non-exported linkonce/weak variables can always be internalized, regardless
+;; of whether they are const or *unnamed_addr.
+; MOD1-INT: @linkonceodrvarInSingleModule = internal global
+; MOD1-INT: @weakodrvarInSingleModule = internal global
+@linkonceodrvarInSingleModule = linkonce_odr dso_local global ptr null, align 8
+@weakodrvarInSingleModule = weak_odr dso_local global ptr null, align 8
+
+;; Function with an alias are resolved to weak_odr in prevailing module, but
+;; not optimized in non-prevailing module (illegal to have an
+;; available_externally aliasee).
 ; MOD1: define weak_odr void @linkonceodrfuncwithalias()
 ; MOD2: define linkonce_odr void @linkonceodrfuncwithalias()
 define linkonce_odr void @linkonceodrfuncwithalias() #0 {
@@ -35,9 +68,9 @@ entry:
   ret void
 }
 
-; Function with an alias are resolved to weak in prevailing module, but
-; not optimized in non-prevailing module (illegal to have an
-; available_externally aliasee).
+;; Function with an alias are resolved to weak in prevailing module, but
+;; not optimized in non-prevailing module (illegal to have an
+;; available_externally aliasee).
 ; MOD1: define weak void @linkoncefuncwithalias()
 ; MOD2: define linkonce void @linkoncefuncwithalias()
 define linkonce void @linkoncefuncwithalias() #0 {
@@ -52,7 +85,8 @@ entry:
   ret void
 }
 ; MOD1: define weak void @linkoncefunc()
-; MOD1-INT: define weak void @linkoncefunc()
+;; New LTO API will use dso_local
+; MOD1-INT: define weak{{.*}} void @linkoncefunc()
 ; MOD2: declare void @linkoncefunc()
 define linkonce void @linkoncefunc() #0 {
 entry:
@@ -71,6 +105,9 @@ entry:
   ret void
 }
 
+;; A linkonce_odr with a single, non-exported, def can be safely
+;; internalized without increasing code size or being concerned
+;; about affecting function pointer equality.
 ; MOD1: define weak_odr void @linkonceodrfuncInSingleModule()
 ; MOD1-INT: define internal void @linkonceodrfuncInSingleModule()
 ; EXPORTED: define weak_odr void @linkonceodrfuncInSingleModule()