[PATCH] D59709: [ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 2 inline comments as done.
tejohnson added a comment.

In D59709#1440303 <https://reviews.llvm.org/D59709#1440303>, @pcc wrote:

> Does this has the correct behaviour in the case where the explicit instantiation is in a regular object file and the implicit instantiation is in bitcode?


Hmm, no. Nor will it work when the explicit instantiation is in a bitcode file without a summary. I can fix these cases by utilizing the VisibleOutsideSummary flag on the GlobalResolution to conservatively set CanAutoHide=false when that is set.

> 
> 
> In D59709#1440173 <https://reviews.llvm.org/D59709#1440173>, @steven_wu wrote:
> 
>> In D59709#1440113 <https://reviews.llvm.org/D59709#1440113>, @tejohnson wrote:
>>
>> > @pcc might have some info here. But I don't think so. We just can't mark hidden without the global visibility of all symbols, as we have here with ThinLTO (because of ELF picking the most hidden one).
>>
> 
> 
> ELF linkers don't do auto-hide because it would require extending ELF to store the auto-hide bit (or information from which it can be recovered) somewhere. We have extended ELF with an address-significance table which tells us whether symbols are `(local_)unnamed_addr`, but we aren't keeping track of whether symbols are `linkonce_odr`.
> 
>> If this is really linker semantics difference, would it be easier if just make the visibility hidden change in D43130 <https://reviews.llvm.org/D43130> be target specific?
> 
> That would mean losing the auto hide optimization on ELF. I don't think we should do that.





================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:202
+  /// Checks if all copies are eligible for auto-hiding (have flag set).
+  bool isCanAutoHide() const;
 };
----------------
pcc wrote:
> `canAutoHide`? (same elsewhere)
Will do.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:984
+      if (NewLinkage == GlobalValue::WeakODRLinkage && GS->second->isCanAutoHide()) {
+        assert(GV.hasLinkOnceODRLinkage() && GV.hasGlobalUnnamedAddr());
         GV.setVisibility(GlobalValue::HiddenVisibility);
----------------
pcc wrote:
> Can't it also be `local_unnamed_addr`?
I don't know but I wanted to maintain the status quo on that - we only did this in the global unnamed addr case before. If it can be changed to local_unnamed_addr that should probably be done as a follow up.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59709/new/

https://reviews.llvm.org/D59709