[PATCH] D18289: Attach profile summary information to module

[David Li via cfe-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:398
@@ -397,2 +397,3 @@
   if (PGOReader) {
     getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
+    auto *SummaryMD = PGOReader->getSummary().getMD(getModule().getContext());
----------------
This line will be removed later once the client code has been updated right?

================
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;
----------------
vsk wrote:
> `getModule().getContext()` -> `VMContext`?
or Module &M = getModule(); 
M is also referenced in previous line

================
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) {
----------------
vsk wrote:
> 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.
That test is actually not as good for testing summary computation as this one. Perhaps we can simplify this test and move this test (removing the clang dump part) into llvm-profdata dir.


http://reviews.llvm.org/D18289