[ThinLTO] Correctly resolve linkonce when importing aliasee

[tejohnson]

Summary

When we have an aliasee that is linkonce, while we can't convert
the non-prevailing copies to available_externally, we still need to
convert the prevailing copy to weak. If a reference to the aliasee
is exported, not converting a copy to weak will result in undefined
references when the linkonce is removed in its original module.

Add a new test and update existing tests.

[mehdi_amini]

CC Rafael to hint here.

When we have an aliasee that is linkonce, while we can't convert the non-prevailing copies to available_externally

It is not clear to me why? I understand that the verifier won't like now, but why isn't possible in theory?

Also even if we wouldn't want to relax this in general, for ThinLTO we can always clone the function into an available_externally that would replace the alias.

[mehdi_amini]

lib/LTO/LTO.cpp

123 ↗

(On Diff #76196)

It is not clear to me right now why is it part of this stage. Conceptually it seems that you are solving a "promotion" issue, and not a "weak resolution" thing.

[tejohnson]

In D26076#582320, @mehdi_amini wrote:

CC Rafael to hint here.

When we have an aliasee that is linkonce, while we can't convert the non-prevailing copies to available_externally

It is not clear to me why? I understand that the verifier won't like now, but why isn't possible in theory?

Also even if we wouldn't want to relax this in general, for ThinLTO we can always clone the function into an available_externally that would replace the alias.

Yes we could do this, and we've talked about doing this before (see the comment where we compute the GlobalInvolvedWithAlias set just below in llvm::thinLTOResolveWeakForLinkerInIndex). That is a longer term enhancement to handle this limitation of not being able to convert non-prevailing aliasees to available_externally. But note this patch doesn't affect that case, it is rather dealing with a missing case for the prevailing copy.

lib/LTO/LTO.cpp

123 ↗

(On Diff #76196)

The code below here converts the prevailing linkonce to weak to ensure one copy is kept - even if there are no other copies that will be converted to available_externally. This is not new and ensures if we export a reference to that linkonce that it will be satisfied at link time (so that particular part of this routine is in fact like a promotion).

The change in this patch is to allow that part to occur (the correctness part) for an aliasee. It was a missing case, and was resulting in some undefs at link time in certain cases (e.g. the unsats davidxl got when trying to link Clang with ThinLTO + PGO last week).

We could either just better document this part of the handling in this routine, or split it out - e.g a separate pass of promoting linkonce->weak when the linkonce is exported, but that seems orthogonal to fixing the existing handling for this specific case (and somewhat redundant with the linkonce->weak that is done here already).

[tejohnson]

lib/LTO/LTO.cpp

123 ↗

(On Diff #76196)

Note also this routine used to explicitly check for and handle the exported case, but it was removed with the idea that we would always do the linkonce->weak promotion for the prevailing copy (see r274722 and r274784).

[mehdi_amini]

lib/LTO/LTO.cpp

123 ↗

(On Diff #76196)

We could either just better document this part of the handling in this routine, or split it out - e.g a separate pass of promoting linkonce->weak when the linkonce is exported,

Right, splitting it seems better to me.

but that seems orthogonal to fixing the existing handling for this specific case (and somewhat redundant with the linkonce->weak that is done here already).

Agree.

[tejohnson]

lib/LTO/LTO.cpp

123 ↗

(On Diff #76196)

Did you have any more comments on the fix in this patch (which fixes a loophole in the current scheme)?

[mehdi_amini]

Just add a FIXME explaining that there is a correctness part and a compile-time perf part that should be split, and LGTM.

[tejohnson]

In D26076#582984, @mehdi_amini wrote:

Just add a FIXME explaining that there is a correctness part and a compile-time perf part that should be split, and LGTM.

Done.

llvm/trunk/lib/LTO/LTO.cpp

 static void thinLTOResolveWeakForLinkerGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage) {
   for (auto &S : GVSummaryList) {
-    if (GlobalInvolvedWithAlias.count(S.get()))
-      continue;
     GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
     if (!GlobalValue::isWeakForLinker(OriginalLinkage))
       continue;
     // We need to emit only one of these. The prevailing module will keep it,
     // but turned into a weak, while the others will drop it when possible.
+    // This is both a compile-time optimization and a correctness
+    // transformation. This is necessary for correctness when we have exported
+    // a reference - we need to convert the linkonce to weak to
+    // ensure a copy is kept to satisfy the exported reference.
+    // FIXME: We may want to split the compile time and correctness
+    // aspects into separate routines.
     if (isPrevailing(GUID, S.get())) {
       if (GlobalValue::isLinkOnceLinkage(OriginalLinkage))
         S->setLinkage(GlobalValue::getWeakLinkage(
             GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
     }
-    // Alias can't be turned into available_externally.
+    // Alias and aliasee can't be turned into available_externally.
     else if (!isa<AliasSummary>(S.get()) &&
+             !GlobalInvolvedWithAlias.count(S.get()) &&
              (GlobalValue::isLinkOnceODRLinkage(OriginalLinkage) ||
               GlobalValue::isWeakODRLinkage(OriginalLinkage)))
       S->setLinkage(GlobalValue::AvailableExternallyLinkage);
     if (S->linkage() != OriginalLinkage)
       recordNewLinkage(S->modulePath(), GUID, S->linkage());
   }
 }
 

llvm/trunk/test/ThinLTO/X86/Inputs/linkonce_aliasee_ref_import.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+define i32 @main() #0 {
+entry:
+  call void @foo()
+  ret i32 0
+}
+
+declare void @foo()

llvm/trunk/test/ThinLTO/X86/alias_import.ll

 ; PROMOTE-DAG: @linkonceODRfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; Amongst these that are imported, check that we promote only linkonce->weak
 ; PROMOTE-DAG: @linkonceODRfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-DAG: @linkonceODRfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 
 ; These will be imported, check the linkage/renaming after promotion
 ; PROMOTE-DAG: define void @globalfunc()
 ; PROMOTE-DAG: define hidden void @internalfunc.llvm.0()
-; PROMOTE-DAG: define linkonce_odr void @linkonceODRfunc()
+; PROMOTE-DAG: define weak_odr void @linkonceODRfunc()
 ; PROMOTE-DAG: define weak_odr void @weakODRfunc()
-; PROMOTE-DAG: define linkonce void @linkoncefunc()
+; PROMOTE-DAG: define weak void @linkoncefunc()
 ; PROMOTE-DAG: define weak void @weakfunc()
 
 ; On the import side now, verify that aliases to a linkonce_odr are imported, but the weak/linkonce (we can't inline them)
 ; IMPORT-DAG:  declare void @linkonceODRfuncWeakAlias
 ; IMPORT-DAG: declare void @linkonceODRfuncLinkonceAlias
 ; IMPORT-DAG:  @linkonceODRfuncAlias = alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; IMPORT-DAG:  @linkonceODRfuncWeakODRAlias = alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; IMPORT-DAG:  @linkonceODRfuncLinkonceODRAlias = linkonce_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)

llvm/trunk/test/ThinLTO/X86/linkonce_aliasee_ref_import.ll

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/linkonce_aliasee_ref_import.ll -o %t2.bc
+
+; Import with instr limit to ensure only foo imported.
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=main -import-instr-limit=5 %t1.bc %t2.bc
+; RUN: llvm-nm -o - < %t1.bc.thinlto.o | FileCheck %s --check-prefix=NM1
+; RUN: llvm-nm -o - < %t2.bc.thinlto.o | FileCheck %s --check-prefix=NM2
+
+; Import with instr limit to ensure only foo imported.
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:    -r=%t1.bc,foo,pxl \
+; RUN:    -r=%t1.bc,baz,pxl \
+; RUN:    -r=%t1.bc,baz.clone,pxl \
+; RUN:    -r=%t1.bc,bar,pl \
+; RUN:    -r=%t2.bc,main,pxl \
+; RUN:    -r=%t2.bc,foo,l \
+; RUN:    -import-instr-limit=5
+; RUN: llvm-nm -o - < %t1.bc.thinlto.o | FileCheck %s --check-prefix=NM1
+; RUN: llvm-nm -o - < %t2.bc.thinlto.o | FileCheck %s --check-prefix=NM2
+
+; Check that we converted baz.clone to a weak
+; NM1: W baz.clone
+
+; Check that we imported a ref (and not def) to baz.clone
+; NM2: U baz.clone
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+$baz.clone = comdat any
+@baz = weak alias void (), void ()* @baz.clone
+
+define void @foo() #5 align 2 {
+  tail call void @baz.clone()
+  ret void
+}
+define linkonce_odr void @baz.clone() #5 comdat align 2 {
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  ret void
+}
+
+define void @bar() {
+  ret void
+}

llvm/trunk/test/ThinLTO/X86/weak_resolution.ll

 ; MOD2: @linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
 @linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
 
 ; Alias are resolved, but can't be turned into "available_externally"
 ; MOD1: @linkoncealias = weak alias void (), void ()* @linkoncefuncwithalias
 ; MOD2: @linkoncealias = linkonce alias void (), void ()* @linkoncefuncwithalias
 @linkoncealias = linkonce alias void (), void ()* @linkoncefuncwithalias
 
-; Function with an alias are not optimized
-; MOD1: define linkonce_odr void @linkonceodrfuncwithalias()
+; Function with an alias are resolved to weak_odr in prevailing module, but
+; not optimized in non-prevailing module (illegal to have an
+; available_externally aliasee).
+; MOD1: define weak_odr void @linkonceodrfuncwithalias()
 ; MOD2: define linkonce_odr void @linkonceodrfuncwithalias()
 define linkonce_odr void @linkonceodrfuncwithalias() #0 {
 entry:
   ret void
 }
 
-; Function with an alias are not optimized
-; MOD1: define linkonce void @linkoncefuncwithalias()
+; Function with an alias are resolved to weak in prevailing module, but
+; not optimized in non-prevailing module (illegal to have an
+; available_externally aliasee).
+; MOD1: define weak void @linkoncefuncwithalias()
 ; MOD2: define linkonce void @linkoncefuncwithalias()
 define linkonce void @linkoncefuncwithalias() #0 {
 entry:
   ret void
 }
 
 ; MOD1: define weak_odr void @linkonceodrfunc()
 ; MOD2: define available_externally void @linkonceodrfunc()

llvm/trunk/test/tools/gold/X86/thinlto_weak_resolution.ll

 ; are removed by globaldce). FIXME: Need to introduce combined index linkage
 ; that means "drop this function" so we can avoid importing linkonce functions
 ; and drop weak functions.
 ; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck --check-prefix=OPT2 %s
 ; OPT2-NOT: @
 ; OPT2: @weakfunc
 ; OPT2-NOT: @
 
-; RUN: llvm-dis %t.o.4.opt.bc -o - | FileCheck --check-prefix=OPT %s
+; RUN: llvm-dis %t.o.3.import.bc -o - | FileCheck --check-prefix=IMPORT %s
+; RUN llvm-dis %t2.o.3.import.bc -o - | FileCheck --check-prefix=IMPORT2 %s
 
 target triple = "x86_64-unknown-linux-gnu"
 
 
 define i32 @main() #0 {
 entry:
   call void @linkonceodralias()
   call void @linkoncealias()
   call void @linkonceodrfuncwithalias()
   call void @linkoncefuncwithalias()
   call void @linkonceodrfunc()
   call void @linkoncefunc()
   call void @weakodrfunc()
   call void @weakfunc()
   call void @linkonceodrfuncInSingleModule()
   ret i32 0
 }
 
-; Alias are resolved
-; OPT: @linkonceodralias = weak_odr alias void (), void ()* @linkonceodrfuncwithalias
+; Alias are resolved to weak_odr in prevailing module, but left as linkonce_odr
+; in non-prevailing module (illegal to have an available_externally alias).
+; IMPORT: @linkonceodralias = weak_odr alias void (), void ()* @linkonceodrfuncwithalias
+; IMPORT2: @linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
 @linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
 
-; Alias are resolved
-; OPT: @linkoncealias = weak alias void (), void ()* @linkoncefuncwithalias
+; Alias are resolved in prevailing module, but not optimized in
+; non-prevailing module (illegal to have an available_externally alias).
+; IMPORT: @linkoncealias = weak alias void (), void ()* @linkoncefuncwithalias
+; IMPORT2: @linkoncealias = linkonce alias void (), void ()* @linkoncefuncwithalias
 @linkoncealias = linkonce alias void (), void ()* @linkoncefuncwithalias
 
-; Function with an alias are not optimized
-; OPT: define linkonce_odr void @linkonceodrfuncwithalias()
+; Function with an alias are resolved in prevailing module, but
+; not optimized in non-prevailing module (illegal to have an
+; available_externally aliasee).
+; IMPORT: define weak_odr void @linkonceodrfuncwithalias()
+; IMPORT2: define linkonce_odr void @linkonceodrfuncwithalias()
 define linkonce_odr void @linkonceodrfuncwithalias() #0 {
 entry:
   ret void
 }
 
-; Function with an alias are not optimized
-; OPT: define linkonce void @linkoncefuncwithalias()
+; Function with an alias are resolved to weak in prevailing module, but
+; not optimized in non-prevailing module (illegal to have an
+; available_externally aliasee).
+; IMPORT: define weak void @linkoncefuncwithalias()
+; IMPORT2: define linkonce void @linkoncefuncwithalias()
 define linkonce void @linkoncefuncwithalias() #0 {
 entry:
   ret void
 }
 
-; OPT: define weak_odr void @linkonceodrfunc()
+; IMPORT: define weak_odr void @linkonceodrfunc()
+; IMPORT2: define available_externally void @linkonceodrfunc()
 define linkonce_odr void @linkonceodrfunc() #0 {
 entry:
   ret void
 }
-; OPT: define weak void @linkoncefunc()
+; IMPORT: define weak void @linkoncefunc()
+; IMPORT2: define linkonce void @linkoncefunc()
 define linkonce void @linkoncefunc() #0 {
 entry:
   ret void
 }
-; OPT: define weak_odr void @weakodrfunc()
+; IMPORT: define weak_odr void @weakodrfunc()
+; IMPORT2: define available_externally void @weakodrfunc()
 define weak_odr void @weakodrfunc() #0 {
 entry:
   ret void
 }
-; OPT: define weak void @weakfunc()
+; IMPORT: define weak void @weakfunc()
+; IMPORT2: define weak void @weakfunc()
 define weak void @weakfunc() #0 {
 entry:
   ret void
 }
 
-; OPT: weak_odr void @linkonceodrfuncInSingleModule()
+; IMPORT: weak_odr void @linkonceodrfuncInSingleModule()
 define linkonce_odr void @linkonceodrfuncInSingleModule() #0 {
 entry:
   ret void
 }