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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


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


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


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:159
+
+static inline void clearSecFlag(SecHdrTableEntry &Entry, uint64_t Flags) {
+  Entry.Flag &= ~Flags;
----------------
clearSecFlags or 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
----------------
hold->held


================
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,
----------------
held


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68253