[PATCH] D16258: [PGO] Profile Summary Index profile writer and reader support

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thanks, comments inline --


================
Comment at: include/llvm/ProfileData/InstrProf.h:673
@@ +672,3 @@
+  // stored
+  // after the profile header.
+  Version4 = 4,
----------------
Tiny nit, could you move one line up?

================
Comment at: include/llvm/ProfileData/InstrProf.h:709
@@ +708,3 @@
+
+  // Max function entry profile count. This field does not exist for profile
+  // data from IR based instrumentation.
----------------
Nit, do you mind keeping the original comment ('The maximal execution count among all functions.')?

Wdyt of changing getMaxFunctionCount() to return an Optional<uint64_t> in a later commit?

================
Comment at: include/llvm/ProfileData/InstrProf.h:714
@@ +713,3 @@
+  uint64_t MaxBlockCount;
+  uint64_t MaxInternalBlockCount;
+  // Total number of instrumented blocks/edges.
----------------
Missing comment.

================
Comment at: include/llvm/ProfileData/InstrProf.h:726
@@ +725,3 @@
+    return sizeof(Summary) + (NumEntries - 1) * sizeof(Entry);
+  }
+};
----------------
Why not use a SmallVector<uint64_t, N>? Then there's no need for `NumEntries`, `getSize()`, `allocSummary()` etc.

================
Comment at: lib/ProfileData/InstrProf.cpp:642
@@ +641,3 @@
+      NumFunctions(S.TotalNumFunctions) {
+  for (unsigned I = 0; I < S.NumEntries; I++) {
+    const IndexedInstrProf::Summary::Entry &Ent = S.Entries[I];
----------------
A range-based for loop would be nicer here.

================
Comment at: lib/ProfileData/InstrProf.cpp:644
@@ +643,3 @@
+    const IndexedInstrProf::Summary::Entry &Ent = S.Entries[I];
+    DetailedSummary.push_back(
+        {(uint32_t)Ent.Cutoff, Ent.MinBlockCount, Ent.NumBlocks});
----------------
If you add a constructor to ProfileSummaryEntry, you can take advantage of emplace_back() here.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:567
@@ +566,3 @@
+    uint32_t SummarySize = IndexedInstrProf::Summary::getSize(NEntries);
+    std::unique_ptr<IndexedInstrProf::Summary> Summary =
+        IndexedInstrProf::allocSummary(SummarySize);
----------------
In this scenario I'd prefer using a different name to avoid confusion and extra 'this->'s. Maybe 'SummaryInHE'?

================
Comment at: lib/ProfileData/InstrProfReader.cpp:582
@@ +581,3 @@
+                                  &SummaryCutoffs[NumSummaryCutoffs]);
+    this->Summary = llvm::make_unique<ProfileSummary>(Cutoffs);
+    this->Summary->getDetailedSummary();
----------------
As written, constructing the `Cutoffs` vector takes linear time. Passing it into the ProfileSummary constructor is also linear time. ISTM the ProfileSummary constructor shouldn't accept a vector of cutoffs, it should use `ArrayRef`.

If you agree, I can take care of it in a separate patch.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:583
@@ +582,3 @@
+    this->Summary = llvm::make_unique<ProfileSummary>(Cutoffs);
+    this->Summary->getDetailedSummary();
+    return Cur;
----------------
Ignoring the result of a getter is a bit confusing. Maybe you could call `computeDetailedSummary()` directly?

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:78
@@ -77,2 +77,3 @@
 static support::endianness ValueProfDataEndianness = support::little;
+static ProfileSummary *TheProfileSummary = NULL;
 
----------------
These static variables break the use-case of running llvm in a multi-threaded environment! If you know of any other instances like this, they need to be fixed.

Please pass a summary pointer into `EmitData` directly.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:194
@@ +193,3 @@
+    TheSummary->Entries[I].NumBlocks = Res[I].NumBlocks;
+  }
+}
----------------
Using a vector-like container for `Entries` turns this loop into a `resize()` + `std::copy`.


http://reviews.llvm.org/D16258