[PATCH] D29734: IR: Function summary extensions for whole-program devirtualization pass.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h:253
+  /// An "identifier" for a virtual function. This contains the type identifier
+  /// represented as a GUID and the offset from the address point to the virtual
+  /// function pointer.
----------------
mehdi_amini wrote:
> `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?
"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?


================
Comment at: llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h:266
+    std::vector<uint64_t> Args;
+  };
+
----------------
mehdi_amini wrote:
> The plan being to decorate edges in general with constant arguments, there is potential for code-sharing here.
Sure, we can share any appropriate code once the constant arguments on edges feature lands.


================
Comment at: llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h:288
+  /// all constant integer arguments.
+  std::vector<ConstVCall> TypeTestAssumeConstVCalls, TypeCheckedLoadConstVCalls;
 
----------------
mehdi_amini wrote:
> 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?
Makes sense. I'll do that in a followup.


================
Comment at: llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h:355
+  }
+  static inline FunctionSummary::VFuncId getTombstoneKey() {
+    return {0, uint64_t(-2)};
----------------
mehdi_amini wrote:
> inline is the default here, isn't it?
Yes, I'll remove these.


================
Comment at: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp:4854
+  std::vector<FunctionSummary::ConstVCall> PendingTypeTestAssumeConstVCalls,
+      PendingTypeCheckedLoadConstVCalls;
 
----------------
mehdi_amini wrote:
> Document.
Will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D29734