ThinLTO: Remove check for multiple modules before applying weak resolutions.

[pcc]

Summary

This check is not only unnecessary, it can produce the wrong result. If we
are linking a single module and it has an exported linkonce symbol, we need
to promote to weak in order to avoid PR19901-style problems.

[tejohnson]

Is this possible outside of this kind of test case? If there is an exported symbol, then presumably we are linking with another bitcode module (the importing module). In the case of PR19901 the overridden copy was in a native object, but here the importing module must also be bitcode (otherwise we can't actually import the symbol reference and no issue would arise).

[mehdi_amini]

libLTO is handling this differently by adding to llvm.compiler.used, for the same reason as mentioned in another review: linkonce -> weak is (was?) pessimizing codegen on MachO.

[pcc]

If there is an exported symbol, then presumably we are linking with another bitcode module (the importing module).

I mean exported in the sense of "not internalized". This applies in the case where the symbol is used by the native module, in which case we need to make sure we export it here.

libLTO is handling this differently by adding to llvm.compiler.used, for the same reason as mentioned in another review: linkonce -> weak is (was?) pessimizing codegen on MachO.

Okay, but I'm sure you don't just want this to happen in the case where there is a single ThinLTO module. As the test I am adding in D21915 shows, we are already upgrading to weak.

[tejohnson]

In D21917#472497, @pcc wrote:

If there is an exported symbol, then presumably we are linking with another bitcode module (the importing module).

I mean exported in the sense of "not internalized". This applies in the case where the symbol is used by the native module, in which case we need to make sure we export it here.

Ah, ok. I was confused by the different uses of the term "exported" in ThinLTO mode. =(

libLTO is handling this differently by adding to llvm.compiler.used, for the same reason as mentioned in another review: linkonce -> weak is (was?) pessimizing codegen on MachO.

Okay, but I'm sure you don't just want this to happen in the case where there is a single ThinLTO module. As the test I am adding in D21915 shows, we are already upgrading to weak.

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

[pcc]

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

To be more clear, I mean that we're already upgrading to weak in the multiple module case. As far as I'm concerned, that's the most important case here, as presumably if someone is using LTO their program has multiple modules. So I think there's no great harm in upgrading to weak in the single module case.

Probably the most explicit instance of this is linkoncefunc in weak_resolution, which I've now added a promote+internalize test case for.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

That doesn't seem to be necessary. We just need a way to express "auto-hide + keep". There's already a way to do that which is compatible with Mach-O linkers, which is to use linkonce_odr + local_unnamed_addr + llvm.compiler.used. ELF linkers don't care about this (at least unless/until we extend ELF to have an auto-hide bit like Mach-O), so we can just do the same thing there. But that's outside of the scope of what I'm doing here.

[pcc]

• Refresh

[mehdi_amini]

In D21917#476048, @pcc wrote:

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

To be more clear, I mean that we're already upgrading to weak in the multiple module case. As far as I'm concerned, that's the most important case here, as presumably if someone is using LTO their program has multiple modules. So I think there's no great harm in upgrading to weak in the single module case.

Probably the most explicit instance of this is linkoncefunc in weak_resolution, which I've now added a promote+internalize test case for.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

That doesn't seem to be necessary.

Agreed.

We just need a way to express "auto-hide + keep". There's already a way to do that which is compatible with Mach-O linkers, which is to use linkonce_odr + local_unnamed_addr + llvm.compiler.used. ELF linkers don't care about this (at least unless/until we extend ELF to have an auto-hide bit like Mach-O), so we can just do the same thing there. But that's outside of the scope of what I'm doing here.

Agreed as well.

Did you already implemented some pre-LTO hiding based on local_unnamed_addr?

[pcc]

Did you already implemented some pre-LTO hiding based on local_unnamed_addr?

Yes, lld is already using canBeOmittedFromSymbolTable for that purpose.

[tejohnson]

In D21917#476244, @mehdi_amini wrote:

In D21917#476048, @pcc wrote:

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

To be more clear, I mean that we're already upgrading to weak in the multiple module case. As far as I'm concerned, that's the most important case here, as presumably if someone is using LTO their program has multiple modules. So I think there's no great harm in upgrading to weak in the single module case.

Probably the most explicit instance of this is linkoncefunc in weak_resolution, which I've now added a promote+internalize test case for.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

That doesn't seem to be necessary.

Agreed.

We just need a way to express "auto-hide + keep". There's already a way to do that which is compatible with Mach-O linkers, which is to use linkonce_odr + local_unnamed_addr + llvm.compiler.used. ELF linkers don't care about this (at least unless/until we extend ELF to have an auto-hide bit like Mach-O), so we can just do the same thing there. But that's outside of the scope of what I'm doing here.

Agreed as well.

Ok SGTM, I didn't realized the Mach-O case was addressed.

[mehdi_amini]

In D21917#476314, @tejohnson wrote:

In D21917#476244, @mehdi_amini wrote:

In D21917#476048, @pcc wrote:

But that's outside of the scope of what I'm doing here.

Agreed as well.

Ok SGTM, I didn't realized the Mach-O case was addressed.

It is not addressed, but I'll address it more generally at a later point.

lib/LTO/LTO.cpp

 // referencing them because of the import. We make sure we always emit at least
 // one copy.
 void thinLTOResolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage) {
-  if (Index.modulePaths().size() == 1)
-    // Nothing to do if we don't have multiple modules
-    return;
-
   // We won't optimize the globals that are referenced by an alias for now
   // Ideally we should turn the alias into a global and duplicate the definition
   // when needed.
   DenseSet<GlobalValueSummary *> GlobalInvolvedWithAlias;
   for (auto &I : Index)
     for (auto &S : I.second)
       if (auto AS = dyn_cast<AliasSummary>(S.get()))
         GlobalInvolvedWithAlias.insert(&AS->getAliasee());

test/ThinLTO/X86/weak_resolution_single.ll

+; RUN: opt -module-summary %s -o %t.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t2.bc %t.bc
+
+; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t2.bc -exported-symbol=foo -o - | llvm-lto -thinlto-action=internalize -thinlto-module-id=%t.bc - -thinlto-index=%t2.bc -exported-symbol=foo -o - | llvm-dis -o - | FileCheck %s
+
+; CHECK: define weak_odr void @foo()
+define linkonce_odr void @foo() {
+  ret void
+}