[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

1 files changed, 50 insertions, 9 deletions

diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index fa3e060..9f3bff5 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -450,6 +450,12 @@ static void thinLTOInternalizeAndPromoteGUID(
     ValueInfo VI, function_ref<bool(StringRef, ValueInfo)> isExported,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing) {
+  auto ExternallyVisibleCopies =
+      llvm::count_if(VI.getSummaryList(),
+                     [](const std::unique_ptr<GlobalValueSummary> &Summary) {
+                       return !GlobalValue::isLocalLinkage(Summary->linkage());
+                     });
+
   for (auto &S : VI.getSummaryList()) {
     // First see if we need to promote an internal value because it is not
     // exported.
@@ -480,15 +486,50 @@ static void thinLTOInternalizeAndPromoteGUID(
     if (GlobalValue::isInterposableLinkage(S->linkage()) && !IsPrevailing)
       continue;
 
-    // Functions and read-only variables with linkonce_odr and weak_odr linkage
-    // can be internalized. We can't internalize linkonce_odr and weak_odr
-    // variables which are both modified and read somewhere in the program
-    // because reads and writes will become inconsistent.
-    auto *VarSummary = dyn_cast<GlobalVarSummary>(S->getBaseObject());
-    if (VarSummary && !VarSummary->maybeReadOnly() &&
-        !VarSummary->maybeWriteOnly() &&
-        (VarSummary->linkage() == GlobalValue::WeakODRLinkage ||
-         VarSummary->linkage() == GlobalValue::LinkOnceODRLinkage))
+    // Non-exported functions and variables with linkonce_odr or weak_odr
+    // 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
+    // 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
+    // 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.
+    // 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
+    // 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))
       continue;
 
     S->setLinkage(GlobalValue::InternalLinkage);