ThinLTO and linkonce_odr + unnamed_addr

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

Hi Steven,

I’d prefer not to inhibit importing. I am also concerned about putting these symbols in the llvm.compiler_used (I don’t recall earlier discussion around this, but it seems like it could have effects on optimization as you mention).

What are the downsides of #2 (adding visibility hidden)? We already do this when promoting internal linkage to external due to importing. I’m not an expert on how this would affect link semantics.

Thanks,
Teresa

Hi Steven,

I’d prefer not to inhibit importing. I am also concerned about putting these symbols in the llvm.compiler_used (I don’t recall earlier discussion around this, but it seems like it could have effects on optimization as you mention).

What are the downsides of #2 (adding visibility hidden)? We already do this when promoting internal linkage to external due to importing. I’m not an expert on how this would affect link semantics.

For macho, this should be a straight up improvement. It eliminates the weak external, which is big win, but it doesn’t solve other side affects for this promotion, i.e, the promoted symbol cannot be dropped by compiler or linker anymore if they are not used.

We also need to make sure the linker is doing the right thing when coming into merging visibility. When linker sees two weak symbols, one is hidden, one is default, I think the correct semantic is to take the default visibility. Same goes for unnamed_addr, quote Language Reference “Note that a constant with significant address can be merged with a unnamed_addr constant, the result being a constant whose address is significant.” I did a quick experiment yesterday. ld64 behaves correctly for visibility but llvm-link does not. I can fix that.

If other targets can all agree on this behavior, fix #2 should not have any downside comparing to current implementation. We just need to find some other way to improve code size.

Steven

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?

Hi,

My understanding is also that there should be no semantic difference between linkonce_odr and weak_odr other than the discardable aspect.

So I don’t understand why llvm.compiler_used would be a problem here? It seems to me that linkonce_odr + llvm.compiler_used is exactly like weak_odr, isn’t it?

Cheers,

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? :slight_smile:

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

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.

I agree with Teresa, we should probably do #2 to preserve behavior for now.

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?

I didn’t realize that that WeakDefCanBeHiddenDirective is only available on Darwin. So if we are doing it for #2, it should be a Darwin only fix as well.

Steven

Isn’t #2 actually changing the behavior by changing the visibility while llvm.compiler_used seems actually closest to current behavior to me?

The visibility hidden would break comparison of function as you mentioned, which is not the case with auto-hide if I didn’t miss anything.

Something I haven’t seen mentioned: why are we dropping the unnamed_addr? Can’t we preserve it with the weak symbol? Would it be OK to add auto-hide in this case (maybe it would already happen)?
Can we use the new local_unnamed_addr that was added (by pcc or Rafael I don’t remember)? I think this attribute matches exactly the auto-hide semantic. Wasn’t the idea that this could be added any time by a module-level infer_attribute pass?

We didn’t drop unnamed_addr. I just don’t think weakodr_addr + unnamed_addr is safe to hide in all cases.

I don’t know if I interpreted local_unnamed_addr correctly but I think it is mostly the same in thinLTO for ld64. The code generator only looks at the individual module and ld64 will be in charge of merging all the symbols with autohide. It doesn’t really help in this case. But it is interesting in general because according to the definition for local_unnamed_addr, the symbol has to be linkonce_odr to be auto hide as well. ThinLTO promotion can break that as well.

Steven

But it is interesting in general because according to the definition for local_unnamed_addr, the symbol has to be linkonce_odr to be auto hide as well. ThinLTO promotion can break that as well.

But it is interesting in general because according to the definition for local_unnamed_addr, the symbol has to be linkonce_odr to be auto hide as well. ThinLTO promotion can break that as well.

I don’t think there is such guarantees. The description doesn’t prevent local_unnamed_addr on other linkage types. If your assumption is correct, then the description can simply state "symbols with local_unnamed_addr can be hidden from the symbol table, regardless of the linkage type). I guess I will leave pcc to interpret this.

If my interpretation is correct, then a constant with linkonce_odr + local_unnamed_addr can be hidden from symbol table before promotion. After promotion, it no longer satisfies rule 2, so it has be in the symbol table.

Steven

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. :slight_smile:

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?

I don't see what would justify this, but I can miss some subtleties here.
If we can't do this with promotion, then it would be very unfortunate: the
whole point of these was to allow to "auto-hide".

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,

I think thinLTO should handle unnamed_addr and generate auto hide if needed. We can put (local_)unnamed_addr into GlobalSummary and teach thinLTO to add visibility hidden for symbols that satisfies the condition. I don’t think this is very hard to do.

I don’t know if we have a definition for unnamed_addr. Do we treat it local_unnamed_addr that automatically satisfy condition #1? Then promote linkonce_odr + unnamed_addr into weak_odr + unnamed_addr + hidden is a correct transform then.

Steven

is a correct transform then.

I believe it is only possible to hide if the symbol isn't required to be
preserved by the linker.
But in this case we should always be able to hide regardless of the linkage
don't we?