[PATCH] D66374: [SampleFDO] Add symbol whitelist in the profile and use it when profile-sample-accurate is enabled

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.
Herald added a subscriber: aaron.ballman.


================
Comment at: include/llvm/ProfileData/SampleProf.h:121
+struct SecHdrTableEntry {
+  SecType Type;
+  uint64_t Flag;
----------------
I'd initialize these to something, and perhaps have an invalid value for the Type you can use here for initialization. This type is used in a std::vector; so it'd be possible for one to write at a point "vector.resize()" and end up with garbage.


================
Comment at: include/llvm/ProfileData/SampleProf.h:576
+/// in the profile and a function newly added.
+class SymbolWhiteList {
+public:
----------------
SymbolAllowList? (may be easier to understand)


================
Comment at: include/llvm/ProfileData/SampleProf.h:580
+  /// for the input Name.
+  void add(StringRef Name, bool copy = false) {
+    if (!copy) {
----------------
Copy not copy


================
Comment at: include/llvm/ProfileData/SampleProf.h:588
+
+  bool contains(StringRef Name) { return Syms.count(Name); }
+
----------------
bool contains(..) const

same for size()


================
Comment at: include/llvm/ProfileData/SampleProf.h:595
+
+  unsigned size() { return Syms.size(); }
+
----------------
why not return size_t,  which is what Syms.size() is?


================
Comment at: include/llvm/ProfileData/SampleProf.h:597
+
+  std::error_code read(uint64_t CompressSize, uint64_t UncompressSize,
+                       const uint8_t *Data);
----------------
Nit: why not just pass a StringRef with length CompressSize, and then rename UncompressSize to ExpectedUncompressedSize - I think it would clarify the responsibility of the function


================
Comment at: include/llvm/ProfileData/SampleProf.h:604
+private:
+  DenseSet<StringRef> Syms;
+  BumpPtrAllocator Allocator;
----------------
is Syms expected to be large? IIUC, DenseSet is good for small sets, it's "quadratically probed". (https://llvm.org/docs/ProgrammersManual.html#llvm-adt-denseset-h)


================
Comment at: include/llvm/ProfileData/SampleProf.h:605
+  DenseSet<StringRef> Syms;
+  BumpPtrAllocator Allocator;
+};
----------------
can you add a comment explaining the role of Allocator? I think it answers my question about ownership below, too.


================
Comment at: include/llvm/ProfileData/SampleProfReader.h:499
+
+  std::error_code readSymbolWhiteList();
+
----------------
nit: I would find "AllowList" easier to understand (less term decoding)


================
Comment at: include/llvm/ProfileData/SampleProfWriter.h:59
 
+  virtual void setSymbolWhiteList(SymbolWhiteList *WL) {}
+
----------------
should this be passed in the ctor? also, if not, why virtual - would it ever do anything else than set a field (tradeoff: less virtual calls, for extra pointer field)


================
Comment at: include/llvm/ProfileData/SampleProfWriter.h:83
+  /// Profile format.
+  SampleProfileFormat Format;
 };
----------------
this could be const if Format were passed in the ctor - would Format ever mutate?


================
Comment at: include/llvm/ProfileData/SampleProfWriter.h:162
+
+  uint64_t FileStart;
+  uint64_t SecHdrTableOffset;
----------------
Please init field values - safe to do here, at definition (maintainability - wise)


================
Comment at: lib/ProfileData/SampleProf.cpp:204
+                              CompressSize);
+  char *Buffer = Allocator.Allocate<char>(UncompressSize);
+  llvm::Error E = zlib::uncompress(CompressedStrings, Buffer, UncompressSize);
----------------
who takes ownership of Buffer?


================
Comment at: lib/ProfileData/SampleProfReader.cpp:497
+    case SecFuncProfile:
+      while (Data < BufStart + Entry.Offset + Entry.Size) {
+        if (std::error_code EC = readFuncProfile())
----------------
nit: define BufStart + Entry.Offset + EntrySize as EntryEnd, and then reuse below, too (easier to understand)


================
Comment at: lib/ProfileData/SampleProfReader.cpp:623
+
+  for (uint32_t i = 0; i < (*EntryNum); i++)
+    if (std::error_code EC = readSecHdrTableEntry())
----------------
I not i

also, ++

why uint32_t and not size_t?


================
Comment at: lib/ProfileData/SampleProfReader.cpp:1175
   FunctionSamples::Format = Reader->getFormat();
-  if (std::error_code EC = Reader->readHeader())
+  if (std::error_code EC = Reader->readHeader()) {
     return EC;
----------------
why the braces? seems unrelated


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:287
+  SecHdrTableOffset = OutputStream->tell();
+  for (uint32_t i = 0; i < SecNumber; i++) {
+    Writer.write(static_cast<uint64_t>(-1));
----------------
I not i
++I
why not size_t?

same below


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:300
+  // Set OutputStream to the location saved in SecHdrTableOffset.
+  if (OFS.seek(SecHdrTableOffset) == (uint64_t)-1)
+    return sampleprof_error::ostream_seek_unsupported;
----------------
static_cast<uint64_t>?


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:304
+
+  for (uint32_t i = 0; i < SecHdrTable.size(); i++) {
+    Writer.write(static_cast<uint64_t>(SecHdrTable[i].Type));
----------------
i->I, ++I


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:312
+  // Reset OutputStream.
+  if (OFS.seek(Saved) == (uint64_t)-1)
+    return sampleprof_error::ostream_seek_unsupported;
----------------
static_cast?


================
Comment at: lib/ProfileData/SampleProfWriter.cpp:490
 
+  Writer->Format = Format;
   return std::move(Writer);
----------------
would format passing make sense in the ctor of the profile writer? it's logically a const field of that, correct?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66374