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

David Li via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 1 11:17:47 PST 2016

davidxl added inline comments.

Comment at: include/llvm/ProfileData/InstrProf.h:751
@@ +750,3 @@
+  // Reserved fields for future
+  uint64_t Reserved[5];
+  /// Number of Cutoff entries
xur wrote:
> vsk wrote:
> > I'm not sure about this Reserved field. IMO any new future functionality should come with a format version bump, even if the on-disk layout stays the same. That means we can teach the reader to skip straight to NumEntries for v4 files.
> Do we need to use uint64_t for TotalNumFunctions and TotalNumBlocks. Will uint32_t enough? 
Yes it is true that the version number will always bump up when new functionality is added -- however keeping the same layout can greatly simplify the reader change. 

To remove the reserved field, I think it might be better to split the summary into two parts: summary header plus the Summary entry array. I will make that change and remove the reserved fields.

Comment at: lib/ProfileData/InstrProfWriter.cpp:197
@@ +196,3 @@
+    TheSummary->Entries[I].NumBlocks = Res[I].NumBlocks;
+  }
vsk wrote:
> I see. Is it possible to make the Entries array an array of ProfileSummaryEntry? We'd have to make the Cutoff field 64-bit, but it would help keep things in sync. That also lets us use the default copy-constructor here, i.e `TheSummary->Entries[I] = Res[I]`.
It is tempting to do that -- but ProfileSummaryEntry data structure is intended to also be shared with sample PGO and its contents may be subject to unrelated changes in the future which may affect layout. I would like to avoid it.


More information about the llvm-commits mailing list