Page MenuHomePhabricator

[ThinLTO] Implement index-based WPD
Needs ReviewPublic

Authored by tejohnson on Nov 30 2018, 4:25 PM.

Details

Reviewers
pcc
davidxl
Summary

This patch adds support to the WholeProgramDevirt pass to perform
index-based WPD, which is invoked from ThinLTO during the thin link.

The ThinLTO backend (WPD import phase) behaves the same regardless of
whether the WPD decisions were made with the index-based or (the
existing) IR-based analysis.

Depends on D54815.

Diff Detail

Event Timeline

tejohnson created this revision.Nov 30 2018, 4:25 PM
pcc added a comment.Jan 27 2019, 3:41 PM

Does this correctly handle the case where the only implementation is internal? I didn't see any code handling that. In the existing implementation this is done here: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/WholeProgramDevirt.cpp#814

In D55153#1373111, @pcc wrote:

Does this correctly handle the case where the only implementation is internal? I didn't see any code handling that. In the existing implementation this is done here: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/WholeProgramDevirt.cpp#814

This is a good catch. I suspect I didn't hit it since I am marking all the added call edges as Hot, which means we are highly likely to import them, which will result in them getting promoted. But if we were to ever not import it would cause an issue. Let me see if I can confirm this, and the fix should be straightforward (ensure they are all treated as exported if we have references on type tests in other modules so we mark them as promoted in the index).

In D55153#1373111, @pcc wrote:

Does this correctly handle the case where the only implementation is internal? I didn't see any code handling that. In the existing implementation this is done here: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/WholeProgramDevirt.cpp#814

This is a good catch. I suspect I didn't hit it since I am marking all the added call edges as Hot, which means we are highly likely to import them, which will result in them getting promoted. But if we were to ever not import it would cause an issue. Let me see if I can confirm this, and the fix should be straightforward (ensure they are all treated as exported if we have references on type tests in other modules so we mark them as promoted in the index).

There were actually a few different issues I discovered while fixing this and writing an additional test.

First of all, we need to make note of any devirtualized targets that have type test users in a different module so that they are not subsequently internalized (since the designation of being exported is based on the GlobalResolution's Partition info which was set up based on linker resolution info). To do this we simply pass in the ExportedGUIDs set from the client that we accumulate this info into and update it based on WPD results. This also ensures we promote any local devirtualized targets used in another module.

Second of all, when the devirtualized implementation is internal and it is used by devirtualized call in another module it must be promoted (ensured by the above change), and we need to note the promoted name in the WPDRes SingleImplName. We can simply record the global promoted name in the WPDRes when we detect that we are adding a cross-module devirtualiztion.

Third, when the devirtualized implementation is internal, and all devirtualized uses are in the same module, it still might need to be promoted if it is subsequently exported by cross-module importing. The simplest and most efficient way to handle this that I came up with is to simply record the information necessary to quickly locate the corresponding WPDRes structure for each local devirt target's ValueInfo in the case where it is not yet exported by a devirtualization. Then after cross module importing, we simply check if any of these VIs has been exported by importing, and update the recorded SingleImplName.

All of these cases are checked with an additional test (devirt2.ll) that I have added.

Finally, the new devirt2.ll also checks the existing hybrid WPD for completeness. Since in that case the local targets are promoted and renamed during module splitting, and the name is based on a hash, I added some handling to llvm-lto2.cpp to enable matching of the provided resolutions based on the unpromoted name.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 7:53 AM
tejohnson updated this revision to Diff 185318.Feb 5 2019, 8:29 AM

Handle exported symbols correctly

pcc added a comment.Apr 26 2019, 5:58 PM

Could you please revert the changes from D54815 out of this change so that it is easier to read?

In D55153#1481171, @pcc wrote:

Could you please revert the changes from D54815 out of this change so that it is easier to read?

Yep, sorry, looks like that got messed up when I merged in some other changes awhile back. I'm working on merging in the changes I made this morning and last week to the underlying index patch, and then will update this to contain only the delta. Hopefully it will get updated later this afternoon.

In D55153#1481171, @pcc wrote:

Could you please revert the changes from D54815 out of this change so that it is easier to read?

Yep, sorry, looks like that got messed up when I merged in some other changes awhile back. I'm working on merging in the changes I made this morning and last week to the underlying index patch, and then will update this to contain only the delta. Hopefully it will get updated later this afternoon.

I have rebased on top of the recent changes to D54815, and updated the patch so it reflects the diffs on top of that patch.