This is an archive of the discontinued LLVM Phabricator instance.

IR: Function summary extensions for whole-program devirtualization pass.
ClosedPublic

Authored by pcc on Feb 8 2017, 3:33 PM.

Details

Summary

The summary information includes all uses of llvm.type.test and
llvm.type.checked.load intrinsics that can be used to devirtualize calls,
including any constant arguments for virtual constant propagation.

Event Timeline

pcc created this revision.Feb 8 2017, 3:33 PM
tejohnson added inline comments.Feb 10 2017, 7:47 AM
llvm/include/llvm/IR/ModuleSummaryIndex.h
256

I wonder if GUID should be moved out of GlobalValue since we are now using it for types?

260

Maybe "all constant integer args" to be clearer?

282

Maybe "do not have all constant integer arguments" to be clearer?

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
91

Why arg_begin() + 1? Skipping the "this" pointer? Maybe add a comment.

225–226

This handling is getting long - consider outlining

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
3371

Why no abbrev ids for these record types?

3371

Add comments to new static funcs here and elsewhere

pcc updated this revision to Diff 88030.Feb 10 2017, 11:56 AM
pcc marked 5 inline comments as done.
  • Address review comments
llvm/include/llvm/IR/ModuleSummaryIndex.h
256

Not sure -- it isn't clear where else it should go though. One possibility is ModuleSummaryIndex, but that will also require moving getGUID to avoid a circular dependency. Moving it should probably be a separate cleanup, I reckon.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
3371

Why no abbrev ids for these record types?

It isn't clear that it would be worth it with the current abbrev scheme. VBR would be a good choice for offsets and arguments since they are typically relatively small, but GUIDs are evenly distributed so the best choice for those would be Fixed.

So a good abbrev for *_VCALLS would involve an array of (Fixed, VBR) pairs, but I don't think we can express that at the moment. And a good abbrev for *_CONST_VCALL would use an array of VBR. But that is basically just the encoding we use at the moment for unabbreviated records.

tejohnson accepted this revision.Feb 10 2017, 2:24 PM
tejohnson added inline comments.
llvm/include/llvm/Bitcode/LLVMBitCodes.h
221

Add the "all constant" to comments for records too.

llvm/include/llvm/IR/ModuleSummaryIndex.h
256

I'm not sure either, and it doesn't have to be done in this patch, but it stood out to me since we are adding more non-GV uses of this functionality. The summary is not the right place, since getGUID and GUID are also used in the instrumentation based PGO functionality.

This revision is now accepted and ready to land.Feb 10 2017, 2:24 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Feb 10 2017, 5:36 PM
llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h
253 ↗(On Diff #88056)

address point?

Beyond the typo, I have no idea reading this what is the address pointer referring to? Is it the address of the vtable?

266 ↗(On Diff #88056)

The plan being to decorate edges in general with constant arguments, there is potential for code-sharing here.

288 ↗(On Diff #88056)

How much do we grow the size of index entries? It seems that we're adding 5 vectors while in the general case this feature is unused.
Can we get an auxiliary structure of some sort?

355 ↗(On Diff #88056)

inline is the default here, isn't it?

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
4854 ↗(On Diff #88056)

Document.

pcc added inline comments.Feb 10 2017, 5:51 PM
llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h
253 ↗(On Diff #88056)

"address point" is as defined here: https://mentorembedded.github.io/cxx-abi/abi.html#vtable-general

I can include a reference to that document in the comment perhaps?

266 ↗(On Diff #88056)

Sure, we can share any appropriate code once the constant arguments on edges feature lands.

288 ↗(On Diff #88056)

Makes sense. I'll do that in a followup.

355 ↗(On Diff #88056)

Yes, I'll remove these.

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
4854 ↗(On Diff #88056)

Will do.

mehdi_amini added inline comments.Feb 10 2017, 6:09 PM
llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h
253 ↗(On Diff #88056)

I'm a big fan of adding reference to external document/standard in comments indeed. That helps readers that aren't as familiar with the standard. Thks.

pcc added a comment.Feb 10 2017, 7:30 PM

I addressed the post-commit review comments in r294822.