[PATCH] D68253: [SampleFDO] Add compression support for any section in ExtBinary profile format

[Wei Mi via Phabricator via llvm-commits]

wmi marked 6 inline comments as done.
wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:148
   SecType Type;
   uint64_t Flag;
   uint64_t Offset;
----------------
davidxl wrote:
> Flag->Flags?
Fixed.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:155
+
+static inline void setSecFlags(SecHdrTableEntry &Entry, uint64_t Flags) {
+  Entry.Flag |= Flags;
----------------
davidxl wrote:
> may be name it 'addSecFlags' ? 'setSecFlags' implies Entry.Flag = Flags;
Fixed.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:159
+
+static inline void clearSecFlag(SecHdrTableEntry &Entry, uint64_t Flags) {
+  Entry.Flag &= ~Flags;
----------------
davidxl wrote:
> clearSecFlags or removeSecFlags?
> 
> 
Used removeSecFlags.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:174
+
+  // We will swap the raw_ostream hold by LocalBufStream and that
+  // hold by OutputStream if we try to add a section which needs
----------------
davidxl wrote:
> hold->held
Fixed.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:178
+  // will be temporarily buffered into the underlying raw_string_ostream
+  // originally hold by LocalBufStream. After the data writing for the
+  // section is completed, compress the data in the local buffer,
----------------
davidxl wrote:
> held
Fixed.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:656
       cl::desc("Compress profile symbol list before write it into profile. "));
+  cl::opt<bool> CompressAllSections(
+      "compress-all-sections", cl::init(false), cl::Hidden,
----------------
davidxl wrote:
> Should this imply compressProfSymlist when true?   Or under what condition do we want to compressAllSections but not profsymList? If such case does not exist, we should remove compress symlist option.
I removed compress symlist option.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68253