[PATCH] D134320: [llvm] Teach whole program devirtualization about relative vtables

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a subscriber: pcc.
tejohnson added a comment.

@pcc is probably the best reviewer for this since it looks like he originally introduced the llvm.load.relative intrinsic for relative vtables and will have good familiarity with that and WPD.

Does this rely on D134687 <https://reviews.llvm.org/D134687> for correctness?

In D134320#3804210 <https://reviews.llvm.org/D134320#3804210>, @leonardchan wrote:

> Feel free to add other reviewers you think may be appropriate.
>
> In regards to testing, my initial idea was to have equivalent tests for relative vtables for each existing test in `llvm/test/Transforms/WholeProgramDevirt/`, but it feels like there's a lot of overlap between the tests I already have in this patch. Lemme know if I should go ahead and make copies for each test or if there's any specific tests I should add here.

The tests you have added are all testing I think pure regular LTO. Given that your changes to findLoadCallsAtConstantOffset which is called from findDevirtualizableCallsForTypeTest, this should also allow your new WPD to kick in for split module aka hybrid Regular + Thin LTO, as it is called when summarizing type tests during summary building here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L187
That is sufficient for that mode's WPD since it only relies on the type tests getting ThinLTO summaries, as the vtables themselves will be in the regular LTO split module.

So please also add tests for this mode. A good way would be in a test like llvm/test/ThinLTO/X86/devirt.ll. Note that this tests both split module aka hybrid LTO as well as pure index only ThinLTO. I added a comment below about why the latter will not yet work with your changes and what would be needed there (that relies on vtables being summarized as well). So unless you add that support in this patch too, you could just add the checking in for the new WPD for the split module case (see %t.o), and add a TODO in for the non-split pure ThinLTO case (see %t2.o).



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1007
     Constant *Ptr = getPointerAtOffset(TM.Bits->GV->getInitializer(),
-                                       TM.Offset + ByteOffset, M);
+                                       TM.Offset + ByteOffset, M, TM.Bits->GV);
     if (!Ptr)
----------------
It looks like this relies on support added to getPointerAtOffset for Swift relative vtables in D107645, and there is a comment in this routine that says "(Swift-specific) relative-pointer support starts here":
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/TypeMetadataUtils.cpp#L162

Should this comment be removed, or does the code need to be adjusted to not be Swift-specific in some way?

Additionally, it would be nice to support this WPD for non-split (index only) ThinLTO. To do so it looks like the equivalent handling to the "swift-only" changes in getPointerAtOffset would need to be added when we compute the vtable summaries in findFuncPointers here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L523-L559

along with the change you are making here to look through DSOLocalEquivalent. Can you add a TODO there if you don't want to add that support in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134320