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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: test/ThinLTO/X86/deadstrip.ll:85
+
+; Make sure we keep @goo alive because one variant of @linkonceodrfuncwithalias
+; is calling it.
----------------
trentxintong wrote:
> tejohnson wrote:
> > 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.
> 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.
> 
> 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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56117/new/

https://reviews.llvm.org/D56117