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.
Details
Diff Detail
- Build Status
Buildable 3886 Build 3886: arc lint + arc unit
Event Timeline
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 |
- 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 |
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. |
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. |
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. |
355 ↗ | (On Diff #88056) | inline is the default here, isn't it? |
llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp | ||
4854 ↗ | (On Diff #88056) | Document. |
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. |
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. |
Add the "all constant" to comments for records too.