Alias can make one (but not all) live, we still need to scan all others if this symbol is reachable
from somewhere else.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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; |
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. |
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. |
test/ThinLTO/X86/deadstrip.ll | ||
---|---|---|
85 ↗ | (On Diff #179610) |
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.