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

[David Li via llvm-commits]

davidxl marked 8 inline comments as done.

================
Comment at: include/llvm/ProfileData/InstrProf.h:726
@@ +725,3 @@
+    return sizeof(Summary) + (NumEntries - 1) * sizeof(Entry);
+  }
+};
----------------
vsk wrote:
> Why not use a SmallVector<uint64_t, N>? Then there's no need for `NumEntries`, `getSize()`, `allocSummary()` etc.
This data structure is used for serialization describing on disk/buffer data. The in-memory data structure is ProfileSummary

================
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];
----------------
vsk wrote:
> A range-based for loop would be nicer here.
The number of entries is unknown statically.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:582
@@ +581,3 @@
+                                  &SummaryCutoffs[NumSummaryCutoffs]);
+    this->Summary = llvm::make_unique<ProfileSummary>(Cutoffs);
+    this->Summary->getDetailedSummary();
----------------
vsk wrote:
> 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.
I did not notice -- I think making the ProfileSummary to take a const reference to vector should do. Or even better to avoid the the copy from Cutoffs to DetailedSummaryCutoffs member, a unique_ptr to the cutoffs can be created by the creator of ProfileSummary and the ownership of the cutoffs can be passed in.

Feel free to improve this part.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:78
@@ -77,2 +77,3 @@
 static support::endianness ValueProfDataEndianness = support::little;
+static ProfileSummary *TheProfileSummary = NULL;
 
----------------
vsk wrote:
> 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.
Fixed.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:194
@@ +193,3 @@
+    TheSummary->Entries[I].NumBlocks = Res[I].NumBlocks;
+  }
+}
----------------
vsk wrote:
> Using a vector-like container for `Entries` turns this loop into a `resize()` + `std::copy`.
The target data structure is serialization/persistence so it is in 'raw' form.


http://reviews.llvm.org/D16258