[ThinLTO] Scan all variants of vague symbol for reachability.

[trentxintong]

Summary

Alias can make one (but not all) live, we still need to scan all others if this symbol is reachable
from somewhere else.

[trentxintong]

Run clang-format.

[grimar]

I think this is perhaps fine, a minor comment is below. Also, I think Teresa should give a final accept, not me :)

lib/Transforms/IPO/FunctionImport.cpp

785 ↗

(On Diff #179601)

You can omit -> bool:

if (llvm::all_of(VI.getSummaryList(),
                 [](const std::unique_ptr<llvm::GlobalValueSummary> &S) {
                   return S->isLive();
                 }))
  return;

[trentxintong]

Address comment from @grimar (Thanks!) and update a test case.

[tejohnson]

The code change seems fine. Suggestion for test case simplification below.

test/ThinLTO/X86/deadstrip.ll

85 ↗

(On Diff #179610)

Do we need goo to test the fix, or can't linkonceodrfuncwithalias be checked directly? The real issue seems to be that the prevailing copy of linkonceodrfuncwithalias in the second module is not kept without the fix. It should be kept and converted to weak_odr resolution, which you could check directly without having it call any functions.

[trentxintong]

test/ThinLTO/X86/deadstrip.ll

85 ↗

(On Diff #179610)

Hi Teresa

The prevailing copy of linkonceodrfuncwithalias in the second module (Input/deadstrip.ll) is kept and converted to weak_odr resolution here.

This fix is trying to address a bug when we do the mark-and-sweep to determine what is live (reachable). When we have an alias (@linkonceodralias in this case), it makes us mark the aliasee (@linkonceodrfuncwithalias in deadstrip.ll) alive and then walk the aliasee's references and calls which is nil here (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L818).

While this is OK, we have a problem if the same symbol is also reachable from somewhere else and some variants of it reference different set of global values, i.e. we would return early if one of the variants is alive without this patch (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L780).

In the test case I have here, the symbol @linkonceodrfuncwithalias is first reached through the alias @linkonceodralias and second reached from @linkonceodrfuncwithalias_caller. I am testing we scan all variants of the symbol the second time its reached, instead of early return because its one instance of it is reached by the alias beforehand.

[tejohnson]

test/ThinLTO/X86/deadstrip.ll

85 ↗

(On Diff #179610)

The prevailing copy of linkonceodrfuncwithalias in the second module (Input/deadstrip.ll) is kept and converted to weak_odr resolution here.

Only with your fix (I tried the test case without the fix to understand the failure mode). Without your fix the copy in the second module is not kept. The second time it is reached during liveness analysis via the call from linkonceodrfuncwithalias_caller the VI is already live due to the first copy so we never mark the second copy as live, and it is removed.

This also has the effect of not marking its callees (or refs) live, but that is a second order effect. If you prefer to keep the call to goo in the test that's fine, but it should also be directly testing that we keep the second (and prevailing) copy of linkonceodrfuncwithalias and that it is weak_odr.

[trentxintong]

Address @tejohnson suggestion, she is right about how the test case can be simplified.

[tejohnson]

LGTM

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

auto visit = [&](ValueInfo VI) {
     // on a slightly different binary might provoke preserving, importing
     // and ultimately promoting calls to functions not linked into this
     // binary, which increases the binary size unnecessarily. Note that
     // if this code changes, the importer needs to change so that edges
     // to functions marked dead are skipped.
     VI = updateValueInfoForIndirectCalls(Index, VI);
     if (!VI)
       return;
-    for (auto &S : VI.getSummaryList())
-      if (S->isLive())
-        return;
+
+    // We need to make sure all variants of the symbol are scanned, alias can
+    // make one (but not all) alive.
+    if (llvm::all_of(VI.getSummaryList(),
+                     [](const std::unique_ptr<llvm::GlobalValueSummary> &S) {
+                       return S->isLive();
+                     }))
 
     // We only keep live symbols that are known to be non-prevailing if any are
     // available_externally, linkonceodr, weakodr. Those symbols are discarded
     // later in the EliminateAvailableExternally pass and setting them to
     // not-live could break downstreams users of liveness information (PR36483)
     // or limit optimization opportunities.
     if (isPrevailing(VI.getGUID()) == PrevailingType::No) {
       bool KeepAliveLinkage = false;

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

 define void @boo() {
   ret void
 }
 
 define void @another_dead_func() {
     call void @dead_func()
     ret void
 }
+
+define linkonce_odr void @linkonceodrfuncwithalias() {
+entry:
+  ret void
+}

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

 ; RUN:   -r %t1.bc,_main,plx \
 ; RUN:   -r %t1.bc,_bar,pl \
 ; RUN:   -r %t1.bc,_dead_func,pl \
 ; RUN:   -r %t1.bc,_baz,l \
 ; RUN:   -r %t1.bc,_boo,l \
 ; RUN:   -r %t1.bc,_live_available_externally_func,l \
 ; RUN:   -r %t1.bc,_live_linkonce_odr_func,l \
 ; RUN:   -r %t1.bc,_live_weak_odr_func,l \
+; RUN:   -r %t1.bc,_linkonceodralias,pl \
+; RUN:   -r %t1.bc,_linkonceodrfuncwithalias,l \
+; RUN:   -r %t1.bc,_linkonceodrfuncwithalias_caller,pl \
 ; RUN:   -r %t2.bc,_baz,pl \
 ; RUN:   -r %t2.bc,_boo,pl \
 ; RUN:   -r %t2.bc,_dead_func,l \
 ; RUN:   -r %t2.bc,_another_dead_func,pl \
+; RUN:   -r %t2.bc,_linkonceodrfuncwithalias,pl \
 ; RUN:   -thinlto-threads=1 \
 ; RUN:   -debug-only=function-import 2>&1 | FileCheck %s --check-prefix=DEBUG --check-prefix=STATS
 ; RUN: llvm-dis < %t.out.1.3.import.bc | FileCheck %s --check-prefix=LTO2
 ; RUN: llvm-dis < %t.out.2.3.import.bc | FileCheck %s --check-prefix=LTO2-CHECK2
 ; RUN: llvm-nm %t.out.1 | FileCheck %s --check-prefix=CHECK2-NM
 
 ; RUN: llvm-bcanalyzer -dump %t.out.index.bc | FileCheck %s --check-prefix=COMBINED
 ; Live, NotEligibleForImport, dso_local, Internal

 ; LTO2: define internal void @bar_internal()
 ; LTO2-NOT: @dead_func()
 ; LTO2-NOT: available_externally {{.*}} @baz()
 
 ; Make sure we didn't internalize @boo, which is reachable via
 ; llvm.global_ctors
 ; CHECK2: define void @boo()
 ; LTO2-CHECK2: define dso_local void @boo()
+
+; Make sure we keep @linkonceodrfuncwithalias in Input/deadstrip.ll alive as it
+; is reachable from @main.
+; LTO2-CHECK2: define weak_odr dso_local void @linkonceodrfuncwithalias() {
+
 ; We should have eventually removed @baz since it was internalized and unused
 ; CHECK2-NM-NOT: _baz
 
 ; The final binary should not contain any of the dead functions,
 ; only main is expected because bar is expected to be inlined and stripped out.
 ; CHECK-NM-NOT: bar
 ; CHECK-NM-NOT: dead
 ; CHECK-NM: T _main

 ; RUN:   -r %t1.bc,_main,plx \
 ; RUN:   -r %t1.bc,_bar,pl \
 ; RUN:   -r %t1.bc,_dead_func,pl \
 ; RUN:   -r %t1.bc,_baz,l \
 ; RUN:   -r %t1.bc,_boo,l \
 ; RUN:   -r %t1.bc,_live_available_externally_func,l \
 ; RUN:   -r %t1.bc,_live_linkonce_odr_func,l \
 ; RUN:   -r %t1.bc,_live_weak_odr_func,l \
+; RUN:   -r %t1.bc,_linkonceodralias,pl \
+; RUN:   -r %t1.bc,_linkonceodrfuncwithalias,l \
+; RUN:   -r %t1.bc,_linkonceodrfuncwithalias_caller,pl \
 ; RUN:   -r %t3.bc,_baz,pl \
 ; RUN:   -r %t3.bc,_boo,pl \
 ; RUN:   -r %t3.bc,_dead_func,l \
-; RUN:   -r %t3.bc,_another_dead_func,pl
+; RUN:   -r %t3.bc,_another_dead_func,pl \
+; RUN:   -r %t3.bc,_linkonceodrfuncwithalias,pl
 ; RUN: llvm-dis < %t4.out.1.3.import.bc | FileCheck %s --check-prefix=CHECK-NOTDEAD
 ; RUN: llvm-nm %t4.out.0 | FileCheck %s --check-prefix=CHECK-NM-NOTDEAD
 
 ; We can't internalize @dead_func because of the use in the regular LTO
 ; partition.
 ; CHECK-NOTDEAD: define dso_local void @dead_func()
 ; We also can't eliminate @baz because it is in the regular LTO partition
 ; and called from @dead_func.

 define weak_odr void @live_weak_odr_func() {
     ret void
 }
 
 define available_externally void @live_available_externally_func() {
     ret void
 }
 
+; This alias will set its base object in this file (linkonceodrfuncwithalias)
+; alive.
+; We want to make sure the @linkonceodrfuncwithalias copy in Input/deadstrip.ll
+; is also scanned when computing reachability.
+@linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
+
+define linkonce_odr void @linkonceodrfuncwithalias() {
+entry:
+  ret void
+}
+
+define void @linkonceodrfuncwithalias_caller() {
+entry:
+  call void @linkonceodrfuncwithalias()
+  ret void
+}
+
+
 define void @main() {
     call void @bar()
     call void @bar_internal()
     call void @live_linkonce_odr_func()
     call void @live_weak_odr_func()
     call void @live_available_externally_func()
+    call void @linkonceodrfuncwithalias_caller()
+    call void @linkonceodralias()
     ret void
 }