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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

In D134320#3827335 <https://reviews.llvm.org/D134320#3827335>, @tejohnson wrote:

> @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?

Yes, for getting LTO working on RV end-to-end. Although since it's just the frontend, it can be landed independently of any llvm changes. My understanding is the frontend just needs to emit the correct metadata which will be consumed by WPD via this patch.

> 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).

I think this updated diff should also cover the split-module case(?)



================
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)
----------------
tejohnson wrote:
> 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?
> 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?

Ah, it looks like it might be more correct to instead handle DSOLocalEquivalent in `getPointerAtOffset`. I suppose this could also allow non-swift relative vtables to be accounted for in GlobalDCE. Yeah I think the comment and function should be adjusted since those code bits are used by more than swift vtables.

> 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?

I *think* the new diff adds the proper support for this in that function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134320