This is an archive of the discontinued LLVM Phabricator instance.

WholeProgramDevirt: Implement export/import support for unique ret val opt.
ClosedPublic

Authored by pcc on Feb 13 2017, 4:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Feb 13 2017, 4:09 PM
tejohnson added inline comments.Mar 9 2017, 8:38 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
392 ↗(On Diff #88275)

Document new functions

765 ↗(On Diff #88275)

There are now a bunch of places where this array is being cleared. Suggest putting this in a helper method called something like CSInfo.markDevirtualized() - it will make it more explicit as to what is going on.

794 ↗(On Diff #88275)

I was trying to prove to myself that Res != nullptr when isExported(), but I am having trouble doing so. isExported() will be true when the CSInfo has SummaryTypeCheckedLoadUsers or SummaryHasTypeTestAssumeUsers - these are set up when (Action == PassSummaryAction::Export). But Res will only be non-null when (Action == PassSummaryAction::Export && isa<MDString>(S.first.TypeID)).

Is there a case where we could reach here when Action == PassSummaryAction::Export but !isa<MDString>(S.first.TypeID)?

llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
33 ↗(On Diff #88275)

Why check these - is there a chance they could be modified if the optimization goes wrong?

llvm/test/Transforms/WholeProgramDevirt/import.ll
40 ↗(On Diff #88275)

Add comment on why the optimization can't/shouldn't be applied here.

pcc updated this revision to Diff 91392.Mar 10 2017, 12:02 PM
pcc marked 2 inline comments as done.
  • Address review comments
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
794 ↗(On Diff #88275)

I think isExported() can only possibly be true for type IDs of type MDString because we only add MDStrings to MetadataByGUID (see http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/WholeProgramDevirt.cpp#1204) and we only ever set SummaryTypeCheckedLoadUsers or SummaryHasTypeTestAssumeUsers on members of MetadataByGUID.

llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
33 ↗(On Diff #88275)

Yes, as with D29846 I want to make sure we aren't taking the general VCP code path.

llvm/test/Transforms/WholeProgramDevirt/import.ll
40 ↗(On Diff #88275)

I refreshed this change past r296948 which added a comment at the top of this function.

tejohnson accepted this revision.Mar 10 2017, 12:16 PM

LGTM

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
794 ↗(On Diff #88275)

Ok thanks, missed that.

This revision is now accepted and ready to land.Mar 10 2017, 12:16 PM
This revision was automatically updated to reflect the committed changes.