[PATCH] D151965: [ThinLTO] Fix internalization decisions for weak/linkonce ODR

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson created this revision.
tejohnson added reviewers: aeubanks, steven_wu.
Herald added subscribers: ormris, hiraditya, inglorion.
Herald added projects: lld-macho, All.
Herald added a reviewer: lld-macho.
tejohnson requested review of this revision.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151965

Files:
  lld/test/MachO/lto-internalize-unnamed-addr.ll
  llvm/lib/LTO/LTO.cpp
  llvm/test/ThinLTO/X86/not-internalized.ll
  llvm/test/ThinLTO/X86/weak_externals.ll
  llvm/test/ThinLTO/X86/weak_resolution.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151965.527719.patch
Type: text/x-patch
Size: 16713 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230602/ec0659f6/attachment.bin>