This is an archive of the discontinued LLVM Phabricator instance.

WholeProgramDevirt: Implement importing for single-impl devirtualization.
ClosedPublic

Authored by pcc on Feb 10 2017, 12:55 PM.

Event Timeline

pcc created this revision.Feb 10 2017, 12:55 PM
pcc updated this revision to Diff 88070.Feb 10 2017, 3:50 PM
  • Change the signature of importResolution for an upcoming change
tejohnson added inline comments.Mar 8 2017, 8:17 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
404

Document new functions.

1053

On the export side there were checks on whether TypeID was an MDString, needed here too?

1058

I forget - is there a constraint that the single implementation function return void and not take any args?

1060

I assume IsExported should never be true since we invoke this on import? Should that be checked?

1095

Why can we skip the rest of the function in import mode? E.g. we won't get an optimization remark emitted. Or is the rest of the code only necessary on the export side? Needs a comment at least.

1162

This guard went away - why?

pcc marked 2 inline comments as done.Mar 8 2017, 2:28 PM
pcc added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1053

No, on the import side all type IDs are MDStrings (ensured by ThinLTOBitcodeWriter).

1058

The type of the function in the declaration is irrelevant because every call site will cast it to the correct type. Added comment.

1095

Yes, the rest of the code is only necessary on the export side or during regular LTO (the remarks as well -- the export phase will emit remarks for the whole program).

1162

It is unnecessary. The map NumUnsafeUsesForTypeTest will be empty if TypeCheckedLoadFunc is null.

pcc updated this revision to Diff 91072.Mar 8 2017, 2:28 PM
  • Address review comments
tejohnson accepted this revision.Mar 8 2017, 4:17 PM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1058

I suppose you could get the correct type then from the IR? Not sure if there is any benefit though...

This revision is now accepted and ready to land.Mar 8 2017, 4:17 PM
pcc added inline comments.Mar 8 2017, 4:27 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1058

Not really. The FunctionType of a declaration is completely unused (or at least it ought to be).

There's also not necessarily one "correct" type because there may be multiple call sites and each call site may use a different type for "this".

This is one of the reasons why I would like to see FunctionType removed.

This revision was automatically updated to reflect the committed changes.