[PATCH] D18289: Attach profile summary information to module

[Vedant Kumar via cfe-commits]

vsk added a comment.

Thanks! Some inline comments --


================
Comment at: lib/CodeGen/CodeGenModule.cpp:399
@@ -398,1 +398,3 @@
     getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
+    auto *SummaryMD = PGOReader->getSummary().getMD(getModule().getContext());
+    // Make sure returned metadata is non-null;
----------------
`getModule().getContext()` -> `VMContext`?

================
Comment at: lib/CodeGen/CodeGenModule.cpp:401
@@ +400,3 @@
+    // Make sure returned metadata is non-null;
+    assert(SummaryMD);
+    getModule().setProfileSummary(SummaryMD);
----------------
I don't think we need this assert. A lot of code depends on `MDTuple::get` just working -- it's fine to crash if it doesn't.

================
Comment at: test/Profile/profile-summary.c:5
@@ +4,3 @@
+// RUN: %clang %s -o - -mllvm -disable-llvm-optzns -emit-llvm -S -fprofile-instr-use=%t.profdata | FileCheck %s
+//
+int begin(int i) {
----------------
ISTM that a lot of the lines in this file do not contribute additional coverage of the changes introduced by your patch.

We already have tests which show that the summary code works (`general.proftext`). I think we just need 1 empty function with 1 counter in this file. We don't need to check that the entire detailed summary appears in the metadata, just one cutoff entry should suffice.


http://reviews.llvm.org/D18289