Page MenuHomePhabricator

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

Authored by trentxintong on Thu, Dec 27, 7:14 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

trentxintong created this revision.Thu, Dec 27, 7:14 PM

Run clang-format.

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;

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

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 marked an inline comment as done.Wed, Jan 2, 1:23 PM
trentxintong added inline comments.
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 added inline comments.Wed, Jan 2, 1:36 PM
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.

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

This revision is now accepted and ready to land.Wed, Jan 2, 3:18 PM
This revision was automatically updated to reflect the committed changes.