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

Fri May 10 10:35:32 PDT 2019

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM

Sorry for the long delay in reviewing this.

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

> I think there might still be a problem where we're dropping the distinction between visible to other translation units and visible outside the linkage unit. This is related to the distinction between LDPR_PREVAILING_DEF_IRONLY and LDPR_PREVAILING_DEF_IRONLY_EXP in the gold plugin interface. Let me look into it a bit more and get back to you.

I don't think this is a problem. After having reminded myself about how things work, I realise that the linkers already keep track of this distinction and reflect it in the VisibleToRegularObj field.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:984
+      if (NewLinkage == GlobalValue::WeakODRLinkage && GS->second->isCanAutoHide()) {
+        assert(GV.hasLinkOnceODRLinkage() && GV.hasGlobalUnnamedAddr());
         GV.setVisibility(GlobalValue::HiddenVisibility);
----------------
tejohnson wrote:
> 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.
Okay, I missed that you were checking for global unnamed_addr when setting the flag to begin with.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59709