[LTO] Simplify internalize logic. NFC

[MaskRay]

Summary

D151965 removed incorrect internalization for {linkonce,weak}{,_odr} when
the prevailing copy is in native code. The multiple conditions are based
on negative conditions, which can be simplified to be based on positive cases:

• an object with an external linkage (must be prevailing) can be internalized
• a prevailing object with a {linkonce,weak}{,_odr} or common linkage can be internalized.

Further, the lengthy comment is a bit misleading, as it doesn't say that
objects with an external/linkonce/weak linkage can be internalized.
Clarify it.

Add non-odr weak/linkonce tests to weak_resolution.ll to improve
coverage.

[tejohnson]

Thanks for cleaning this up, I agree it is easier to follow with the restructuring. lgtm.

llvm/lib/LTO/LTO.cpp

472

Nit: remove braces

[MaskRay]

llvm/lib/LTO/LTO.cpp

472

Ah, there are two statements in the body (with one continue;):)

[tejohnson]

llvm/lib/LTO/LTO.cpp

472

Woops, somehow I overlooked that line given the spacing in Phabricator. Nvm

llvm/lib/LTO/LTO.cpp

if (isExported(S->modulePath(), VI)) {
         S->setLinkage(GlobalValue::ExternalLinkage);
       continue;
     }
 
     // Otherwise, see if we can internalize.
     if (!EnableLTOInternalization)
       continue;
 
-    // Ignore local and appending linkage values since the linker
-    // doesn't resolve them (and there is no need to internalize if this is
-    // already internal).
-    if (GlobalValue::isLocalLinkage(S->linkage()) ||
-        S->linkage() == GlobalValue::AppendingLinkage)
-      continue;
-
-    // We can't internalize available_externally globals because this
-    // can break function pointer equality.
-    if (S->linkage() == GlobalValue::AvailableExternallyLinkage)
-      continue;
-
-    bool IsPrevailing = isPrevailing(VI.getGUID(), S.get());
-
-    if (GlobalValue::isInterposableLinkage(S->linkage()) && !IsPrevailing)
+    // Non-exported objects with external linkage can be internalized.
+    if (GlobalValue::isExternalLinkage(S->linkage())) {
+      S->setLinkage(GlobalValue::InternalLinkage);
       continue;
+    }
 
-    // Non-exported functions and variables with linkonce_odr or weak_odr
+    // Non-exported function and variable definitions with a weak-for-linker
     // linkage can be internalized in certain cases. The minimum legality
     // requirements would be that they are not address taken to ensure that we
     // don't break pointer equality checks, and that variables are either read-
     // or write-only. For functions, this is the case if either all copies are
     // [local_]unnamed_addr, or we can propagate reference edge attributes
     // (which is how this is guaranteed for variables, when analyzing whether
     // they are read or write-only).
     //
-    // However, we only get to this code for weak/linkonce ODR values in one of
+    // However, we only get to this code for weak-for-linkage values in one of
     // two cases:
     // 1) The prevailing copy is not in IR (it is in native code).
     // 2) The prevailing copy in IR is not exported from its module.
     // Additionally, at least for the new LTO API, case 2 will only happen if
     // there is exactly one definition of the value (i.e. in exactly one
     // module), as duplicate defs are result in the value being marked exported.
     // Likely, users of the legacy LTO API are similar, however, currently there
     // are llvm-lto based tests of the legacy LTO API that do not mark
     // duplicate linkonce_odr copies as exported via the tool, so we need
     // to handle that case below by checking the number of copies.
     //
-    // Generally, we only want to internalize a linkonce/weak ODR value in case
+    // Generally, we only want to internalize a weak-for-linker value in case
     // 2, because in case 1 we cannot see how the value is used to know if it
     // is read or write-only. We also don't want to bloat the binary with
-    // multiple internalized copies of non-prevailing linkonce_odr functions.
+    // multiple internalized copies of non-prevailing linkonce/weak functions.
     // Note if we don't internalize, we will convert non-prevailing copies to
     // available_externally anyway, so that we drop them after inlining. The
     // only reason to internalize such a function is if we indeed have a single
     // copy, because internalizing it won't increase binary size, and enables
     // use of inliner heuristics that are more aggressive in the face of a
     // single call to a static (local). For variables, internalizing a read or
     // write only variable can enable more aggressive optimization. However, we
     // already perform this elsewhere in the ThinLTO backend handling for
     // read or write-only variables (processGlobalForThinLTO).
     //
-    // Therefore, only internalize linkonce/weak ODR if there is a single copy,
-    // that is prevailing in this IR module. We can do so aggressively, without
+    // Therefore, only internalize linkonce/weak if there is a single copy, that
+    // is prevailing in this IR module. We can do so aggressively, without
     // requiring the address to be insignificant, or that a variable be read or
     // write-only.
-    if ((S->linkage() == GlobalValue::WeakODRLinkage ||
-         S->linkage() == GlobalValue::LinkOnceODRLinkage) &&
-        // We can have only one copy in ThinLTO that isn't prevailing, if the
-        // prevailing copy is in a native object.
-        (!IsPrevailing || ExternallyVisibleCopies > 1))
+    if (!GlobalValue::isWeakForLinker(S->linkage()) ||
+        GlobalValue::isExternalWeakLinkage(S->linkage()))
       continue;
 
+    if (isPrevailing(VI.getGUID(), S.get()) && ExternallyVisibleCopies == 1)
       S->setLinkage(GlobalValue::InternalLinkage);
   }
 }
 
 // Update the linkages in the given \p Index to mark exported values
 // as external and non-exported values as internal.
 void llvm::thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(StringRef, ValueInfo)> isExported,

llvm/test/ThinLTO/X86/weak_resolution.ll

 ;; 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
 
 ;; 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
+; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc -o - --exported-symbol=_linkonceodrfuncInSingleModule --exported-symbol=_weakfuncInSingleModule | 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,_weakvarInSingleModule,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 %t.bc,_weakfuncInSingleModule,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

 ; MOD1: @linkoncealias = weak alias void (), ptr @linkoncefuncwithalias
 ; MOD2: @linkoncealias = linkonce alias void (), ptr @linkoncefuncwithalias
 @linkoncealias = linkonce alias void (), ptr @linkoncefuncwithalias
 
 ;; 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
+; MOD1-INT: @weakvarInSingleModule = internal global
 @linkonceodrvarInSingleModule = linkonce_odr dso_local global ptr null, align 8
 @weakodrvarInSingleModule = weak_odr dso_local global ptr null, align 8
+@weakvarInSingleModule = weak 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 {
 entry:

 ;; about affecting function pointer equality.
 ; MOD1: define weak_odr void @linkonceodrfuncInSingleModule()
 ; MOD1-INT: define internal void @linkonceodrfuncInSingleModule()
 ; EXPORTED: define weak_odr void @linkonceodrfuncInSingleModule()
 define linkonce_odr void @linkonceodrfuncInSingleModule() #0 {
 entry:
   ret void
 }
+
+; MOD1: define weak void @weakfuncInSingleModule()
+; MOD1-INT: define internal void @weakfuncInSingleModule()
+; EXPORTED: define weak void @weakfuncInSingleModule()
+define weak void @weakfuncInSingleModule() {
+entry:
+  ret void
+}