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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 4 inline comments as done.
wmi added a comment.

In D66513#1639578 <https://reviews.llvm.org/D66513#1639578>, @davidxl wrote:

> 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.


I will add it.

> 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.

My plan was to use different section types to represent different funcProfile sections: secFuncProfile_LBR, secFuncProfile_LBR_LLCM, ... Each will have its own section reader.

> Another question, is multiple such funcProfile section supported?

It is supported. Currently reader only read each section according to section tag. It can be multiple sections with the same type. As long as the section reader supports being called multiple times, there is no problem. (FunctionProfile section reader 'readFuncProfile' can be called multiple times)



================
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
----------------
davidxl wrote:
> extend --> extended
Will fix.


================
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
----------------
davidxl wrote:
> 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.
If we want to append cache misses information, we need to express the parent symbol and line offset again in the form of [line offset: cache miss count], that will introduce redundency because we already express line offset in existing function profile. What we want is to append the information directly in the current format like: [line offset: lbr sample count, cache miss count]. That is why I say we may retire the current function profile section v1 and create a new function profile section v2. But the reader of function profile section v1 can be kept if backward compatibility is required. 

> The format of a new section can even be 'opaque' and the section reader plugin can be provided by the client.

Agreed.




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


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:40
   PF_Compact_Binary,
+  PF_Ext_Binary,
   PF_GCC,
----------------
davidxl wrote:
> is Compact_Ext_Binary also a choice?
Currently Compact_Binary uses md5 to represent string. Compact_Ext_Binary may confuse people to think the ext format also use md5 to represent string. 

In the future, we can port the implementation of Compact_Binary to inherit from Ext_Binary too.  


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66513