[PATCH] D75201: [ThinLTO/LowerTypeTests] Handle unpromoted local type ids

Sun Mar 22 09:04:08 PDT 2020

tejohnson added a comment.

In D75201#1901704 <https://reviews.llvm.org/D75201#1901704>, @tejohnson wrote:

> Thanks for the reproducer!
>
> In D75201#1901357 <https://reviews.llvm.org/D75201#1901357>, @evgeny777 wrote:
>
> > @tejohnson
> >
> > > This should happen if the type ID has no use on type metadata, but then I think it should be removed by WPD.
> >
> > It can't be removed or otherwise handled by WPD, because WPD doesn't even see this type.test (it doesn't exist in the index). The problem is
> >  in the analysis phase of thin LTO compilation. I suggest you take a look at `findDevirtualizableCallsForTypeTest` function and then at 
> >  `findLoadCallsAtConstantOffset`. The latter can process three kind of vptr users: bitcast, load and gep. It can't process store with constant 
> >  expression. So far we don't reflect this type.test in summary index and WPD doesn't see it at all.
>
>
> Ah ok - I misunderstood what reference you were talking about. Even if findDevirtualizableCallsForTypeTest and its descendants could process the reference on the store, it wouldn't automatically help since we are ultimately only looking for uses on virtual calls for devirtualization. And in this case there is no call. As a result WPD does not create a WPDResolution (which defaults to Unknown resolution), and therefore when we import the type id in LTT it appears Unsat.
>
> I think if I expanded findLoadCallsAtConstantOffset to mark all other uses as HasNonCallUses, and when true added the type test to the list of summarized type tests (similar to the way findDevirtualizableCallsForTypeCheckedLoad and its caller in ModuleSummaryAnalysis.cpp behaves for non-call uses), then that might be enough to provoke the correct behavior from LTT.
>
> Hmm, nope that isn't enough to fix it. I tested this change, rebuilt the index by disassembling v2.bc, manually stripping the index, and rebuilding it via "opt -module-summary". Disassembled again and confirmed that the enclosing function's summary now has a type test summary for it. Re-did the link but still see the unreachable in the function after optimization.
>
> This needs more thought and investigation, so at this point I am going to revert the first patch and this follow on one so I can unblock your builds. I may have to table this work for a couple weeks in any case.

I finally had a chance to revisit this, and the solution is pretty straightforward. During WPD when we consider whether to remove the type test assume sequence (which was unconditional before this patch), we should remove them whenever we know that LTT will not get a proper resolution and think they are Unsat. With the earlier version of this patch, I was continuing to remove those sequences when there was no global with the type id, and with the new fixed version of the patch, that is extended to also remove if we are importing and there is no TypeIdSummary resolution, which means there was no vcall (and in that case it isn't useful for later analysis in ICP anyway).

BTW I was able to reproduce this failure (in the same file) on a 2-stage linux self build and test. With the new version of the patch this works correctly with no test failures. I will upload the new version of the patch to D73242 <https://reviews.llvm.org/D73242>, ptal. It includes a test for this situation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75201