This is an archive of the discontinued LLVM Phabricator instance.

WholeProgramDevirt: Implement exporting for single-impl devirtualization.
ClosedPublic

Authored by pcc on Feb 9 2017, 7:48 PM.

Event Timeline

pcc created this revision.Feb 9 2017, 7:48 PM
tejohnson added inline comments.Mar 3 2017, 9:17 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
311

How many of these fields are specific to ThinLTO? I assume at least the isExported method is ThinLTO-specific? It would be good to document that.

Also, as I'm reviewing these patches I'm getting confused as to what aspects of the implementation of the optimization are for ThinLTO, it would be good to include a rough outline of how this works for ThinLTO in this file.

651

Suggest setting IsExported to false if not, so that it will always be set by this routine.

652

Needs comment for this block or at least on why this is being cleared.

679

Do we only ever have IsExported==true and therefore reach here in the ThinLTO case? Please add note to that effect if so.

1171

What does it mean for the TypeID to not be an MDString?

pcc updated this revision to Diff 90551.Mar 3 2017, 4:38 PM
pcc marked 3 inline comments as done.
  • Address review comments
pcc added inline comments.Mar 3 2017, 4:38 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
311

Both of the members I'm adding in this patch are ThinLTO-specific. I've added a comment here and an outline to the top of the file in D29808.

651

We need to leave this field as it is if CSInfo.isExported() is false, because a previous call to this lambda may have set it to true and we need to remember that.

652

Added to the definition of CallSiteInfo in D29808

1171

It generally means that the type ID is a distinct MDNode and is therefore local to the module. This can happen if the merged module contains a module compiled with regular LTO or a module for which we could not generate a module ID (so we weren't able to turn it into an MDString by exporting it -- see http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp#114).

Added test coverage for this case.

This revision is now accepted and ready to land.Mar 3 2017, 4:54 PM
This revision was automatically updated to reflect the committed changes.