[PATCH] D66513: [SampleFDO] Add ExtBinary format to support extension of binary profile

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

llvm-profdata should also have a round trip test:

text format --> ext-format-> text format --> compare text format before and after

text-format-> binary-format->ext-format-> text format --> compare text format.

Also for the the funcProfile section, should it also have a field (in the header) defining the profile kind, such as LBR samples, llc misses etc.   Another question, is multiple such funcProfile section supported?



================
Comment at: include/llvm/ProfileData/SampleProfReader.h:463
+/// SampleProfileReaderExtBinary is a binary format which is more easily to be
+/// extend than SampleProfileReaderRawBinary.
+/// SampleProfileReaderExtBinary format is organized in sections except the
----------------
extend --> extended


================
Comment at: include/llvm/ProfileData/SampleProfReader.h:473
+/// addition to the sample count in the profile body, we can add a new section
+/// with the extension and retire the existing section, and we could choose
+/// to keep the parser of the old section if we want the reader to be able
----------------
why retiring the existing section? A natural way is to append a new section of a new type.  The format of a new section can even be 'opaque' and the section reader plugin can be provided by the client.


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:89
+/// before AddNewSection to mark the correct position.
+void SampleProfileWriterExtBinary::addNewSection(SecType Sec) {
+  uint64_t SectionEnd = OutputStream->tell();
----------------
I think it is better to explicit pass SectionStart as the parameter. It should also return a new offset.


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:101
+
+  markSectionStart();
+  computeSummary(ProfileMap);
----------------
let this function return an offset that can be passed to addNewSection


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:40
   PF_Compact_Binary,
+  PF_Ext_Binary,
   PF_GCC,
----------------
is Compact_Ext_Binary also a choice?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66513/new/

https://reviews.llvm.org/D66513