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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:256
+  struct VFuncId {
+    GlobalValue::GUID GUID;
+    uint64_t Offset;
----------------
I wonder if GUID should be moved out of GlobalValue since we are now using it for types?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:260
+
+  /// A specification for a virtual function call with constant integer
+  /// arguments. This is used to perform virtual constant propagation on the
----------------
Maybe "all constant integer args" to be clearer?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:282
+  /// llvm.assume(llvm.type.test) or llvm.type.checked.load intrinsics that do
+  /// not have constant integer arguments.
+  std::vector<VFuncId> TypeTestAssumeVCalls, TypeCheckedLoadVCalls;
----------------
Maybe "do not have all constant integer arguments" to be clearer? 


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:91
+  std::vector<uint64_t> Args;
+  for (auto &Arg : make_range(Call.CS.arg_begin() + 1, Call.CS.arg_end())) {
+    auto *CI = dyn_cast<ConstantInt>(Arg);
----------------
Why arg_begin() + 1? Skipping the "this" pointer? Maybe add a comment.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:163
         if (CalledFunction->isIntrinsic()) {
-          if (CalledFunction->getIntrinsicID() != Intrinsic::type_test)
-            continue;
-          // Produce a summary from type.test intrinsics. We only summarize
-          // type.test intrinsics that are used other than by an llvm.assume
-          // intrinsic. Intrinsics that are assumed are relevant only to the
-          // devirtualization pass, not the type test lowering pass.
-          bool HasNonAssumeUses = llvm::any_of(CI->uses(), [](const Use &CIU) {
-            auto *AssumeCI = dyn_cast<CallInst>(CIU.getUser());
-            if (!AssumeCI)
-              return true;
-            Function *F = AssumeCI->getCalledFunction();
-            return !F || F->getIntrinsicID() != Intrinsic::assume;
-          });
-          if (HasNonAssumeUses) {
+          switch (CalledFunction->getIntrinsicID()) {
+          case Intrinsic::type_test: {
----------------
This handling is getting long - consider outlining


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3371
 
+static void writeFunctionTypeMetadataRecords(BitstreamWriter &Stream,
+                                             FunctionSummary *FS) {
----------------
Why no abbrev ids for these record types?


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3371
 
+static void writeFunctionTypeMetadataRecords(BitstreamWriter &Stream,
+                                             FunctionSummary *FS) {
----------------
tejohnson wrote:
> Why no abbrev ids for these record types?
Add comments to new static funcs here and elsewhere


https://reviews.llvm.org/D29734