llvm dev - Feb 2018 - ThinLTO and linkonce_odr + unnamed_addr

> From looking at the code, it seems like LLVM is basically opting MachO
into -fvisibility-inlines-hidden all the time, i.e. if the function is
linkonce, it's discardable, so mark it hidden to pretend the compiler
inlined it and discarded it. However, this isn't conforming, because the
addresses of inline functions will no longer compare equal across DSOs.

I think there is a nuance, it is marking these as "auto-hide": it just
means that the address is not taken in the current compilation unit IIRC.
The linker decides that it can hide it in the DSO if none of the
compilation unit is using the address based on this auto-hide flag.

Does it make sense?
-- 
Mehdi


2018-02-07 11:27 GMT-08:00 Reid Kleckner via llvm-dev <
llvm-dev at lists.llvm.org>:
> From looking at the code, it seems like LLVM is basically opting MachO
> into -fvisibility-inlines-hidden all the time, i.e. if the function is
> linkonce, it's discardable, so mark it hidden to pretend the compiler
> inlined it and discarded it. However, this isn't conforming, because
the
> addresses of inline functions will no longer compare equal across DSOs.
>
> Realistically, nobody cares about this guarantee. Maybe we should enable
> -fvisibility-inlines-hidden by default to resolve this inconsistency
> between the platforms. It's a much better out of the box experience
anyway.
>
> On Wed, Feb 7, 2018 at 11:11 AM, Steven Wu <stevenwu at apple.com>
wrote:
>
>> That is a good question and I don't know. The optimization is
>> defined include/llvm/Analysis/ObjectUtils.h. If I enable that for
>> weak_odr + unnamed_addr, no tests are failing so I guess it is a safe
>> optimization? :)
>>
>> It is probably because the autohide optimization is targeted at c++
>> templates and inline functions and we know they have linkonce_odr
linkage,
>> which suggests whoever uses this symbol should have their own copy.
Because
>> the linkonce_odr is safe to drop so it is safe to assume that nothing
else
>> should be relying on the symbol to be available from the current
linkage
>> unit, so it is safe to hide from symbol table. weak_odr is often used
to
>> force to compiler and linker to provide the implementation for template
>> instantiation that is not available in the header. I don't think
they are
>> safe to drop in all cases.
>>
>> Steven
>>
>>
>> On Feb 7, 2018, at 10:29 AM, Reid Kleckner <rnk at google.com>
wrote:
>>
>> There should be no semantic difference between linkonce_odr and
weak_odr,
>> except that weak_odr is non-discardable. Why doesn't the autohide
>> optimization work just as well on weak_odr + unnamed_addr as
linkonce_odr +
>> unnamed_addr?
>>
>> On Tue, Feb 6, 2018 at 5:35 PM, Steven Wu via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> Hi,
>>>
>>> I recently found that thinLTO doesn't deal with globals that
has
>>> linkonce_odr and unnamed_addr (for macho at least) because it
prohibits the
>>> autohide optimization during link time.
>>>
>>> In LLVM, we tagged a global linkonce_odr and unnamed_addr to
indicate to
>>> the linker can hide them from symbol table if they were picked
(aka,
>>> linkonce_odr_auto_hide linkage). It is very commonly used for some
type of
>>> Tables for c++ code in clang for example.
>>> However, thinLTO is promoting these symbols to weak_odr +
unnamed_addr,
>>> which lose the property. As a result, it introduces unnecessary
weak
>>> external symbols and weak external are not good for performance on
darwin
>>> platforms.
>>>
>>> I have few proposed solutions for this issue but I don't know
which one
>>> works the best for none macho platforms and other LTO clients like
lld.
>>>
>>> 1. Use llvm.compiler_used.
>>> As far as I know, the linkage promote are just there to keep the
symbol
>>> through internalize and codegen so adding them to compiler used
should
>>> solve this issue. I was told that there was some objections to do
that in
>>> the first place. Is it because the globals added to compiler used
is
>>> ignored by the optimizer so they cannot be internalized and they
cannot be
>>> optimized away? This works well for the case I am looking at
because c++
>>> VTable can't really be optimized and for darwin platforms
because we can
>>> rely on ld64 to do dead_stripping if needed.
>>>
>>> 2. Add visibility hidden when promote linkonce_odr + unnamed_addr.
>>> Well,this doesn't really preserve the link semantics, but
neither does
>>> promoting linkonce_odr to weak_odr. The global will still end up in
the
>>> symbol table but at least it isn't external so it doesn't
come with a
>>> performance cost.
>>>
>>> 3. We can teach function importer that it cannot just reference to
>>> linkonce_odr + unnamed_addr symbols without importing them. I have
some
>>> thoughts about how to do this so I can propose something if people
are
>>> interested going down this route. I am expecting at least add an
entry in
>>> the global summery and change the cost of importing symbols that
references
>>> to linkonce_odr + unnamed_addr symbols.
>>>
>>> 4. As a temporary fix, just targeting at the VTables for c++. We
can put
>>> a special case for global constants that uses this linkage so they
are
>>> never promoted and their parents are never imported into other
modules. The
>>> benefit for inlining global constants is very minimal and I
don't think we
>>> are doing it currently.
>>>
>>> Let me know if any of those solutions work for other LTO client.
>>>
>>> Thanks
>>>
>>> Steven
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>
>>
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180207/bb8d55c1/attachment.html>

On Wed, Feb 7, 2018 at 11:35 AM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
> > From looking at the code, it seems like LLVM is basically opting MachO
> into -fvisibility-inlines-hidden all the time, i.e. if the function is
> linkonce, it's discardable, so mark it hidden to pretend the compiler
> inlined it and discarded it. However, this isn't conforming, because
the
> addresses of inline functions will no longer compare equal across DSOs.
>
> I think there is a nuance, it is marking these as "auto-hide": it
just
> means that the address is not taken in the current compilation unit IIRC.
> The linker decides that it can hide it in the DSO if none of the
> compilation unit is using the address based on this auto-hide flag.
>
> Does it make sense?
>
Yeah, I missed that. .weak_def_can_be_hidden is a MachO thing that I'm not
familiar with. It's nice that the format supports it. :)

Perhaps instead we should make ThinLTO responsible for doing the
auto-hiding, then? It could do the check of, are all declarations marked
local_unnamed_addr, if so, make it weak_odr + unnamed_addr +
visibility=hidden?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180207/3362dcd9/attachment.html>

2018-02-07 13:23 GMT-08:00 Reid Kleckner <rnk at google.com>:
> On Wed, Feb 7, 2018 at 11:35 AM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
>> > From looking at the code, it seems like LLVM is basically opting
MachO
>> into -fvisibility-inlines-hidden all the time, i.e. if the function is
>> linkonce, it's discardable, so mark it hidden to pretend the
compiler
>> inlined it and discarded it. However, this isn't conforming,
because the
>> addresses of inline functions will no longer compare equal across DSOs.
>>
>> I think there is a nuance, it is marking these as
"auto-hide": it just
>> means that the address is not taken in the current compilation unit
IIRC.
>> The linker decides that it can hide it in the DSO if none of the
>> compilation unit is using the address based on this auto-hide flag.
>>
>> Does it make sense?
>>
>
> Yeah, I missed that. .weak_def_can_be_hidden is a MachO thing that I'm
not
> familiar with. It's nice that the format supports it. :)
>
> Perhaps instead we should make ThinLTO responsible for doing the
> auto-hiding, then? It could do the check of, are all declarations marked
> local_unnamed_addr, if so, make it weak_odr + unnamed_addr +
> visibility=hidden?
>
Possibly, I don't remember the details of the plan: it may depend if the
symbol is required by the linker to be exported outside of the DSO?

This situation (and similar others about linkage changes) is a large part
of what motivated originally the new "resolution" LTO API: it offers a
much
finer grain for the linker to express the constraint around this. The goal
was to be able to make these decisions very accurately regardless of the
situation. For instance a symbol in LTO can be referenced from a non-LTO
object file, but isn't exported outside of the current linkage unit / DSO,
so it does not need to be exported publicly. Unfortunately IIRC this can't
be expressed in the "old" LTO API that is expose in the C interface /
libLTO.

Now for cases where we *already* have unnamed_addr (i.e. not only
local_unnamed_addr) in the IR like here, we should be able to use this
information regardless of the linker resolutions (I think), at least to
generate auto-hide.

Best,

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180207/4b424a4e/attachment.html>