[PATCH] D135929: [profile] Add binary ids into indexed profiles

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:100
+  /// Return a binary id size and data pair.
+  virtual std::pair<uint64_t, const uint8_t *> getBinaryIds() {
+    return std::make_pair<>(0, nullptr);
----------------
Can we use ArrayRef<uint8_t> in lieu of std::pair<uint64_t, const uint8_t *>? Lengths larger than size_t won't work on 32-bit platforms any, since the data is in memory.
This would allow us to use the convenience ArrayRef internally, then convert to 64-bit sizes at the (de)serialization edge.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:78
 
+static size_t RoundUp(size_t size, size_t align) {
+  return (size + align - 1) & ~(align - 1);
----------------
nit: This is alignToPowerOf2 in MathExtras.h.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:383-384
 
-  // Only write out all the fields except 'HashOffset' and 'MemProfOffset'. We
-  // need to remember the offset of these fields to allow back patching later.
-  for (int I = 0; I < N - 2; I++)
+  // Only write out all the fields except 'HashOffset' and 'MemProfOffset' and
+  // 'BinaryIdOffset'. We need to remember the offset of these fields to allow
+  // back patching later.
----------------
nit: 'HashOffset', 'MemProfOffset', and 'BinaryIdOffset'.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:505
+            instrprof_error::malformed,
+            "not enough data to read binary id length");
+      // Write binary id length.
----------------
It's a bit odd that binary ID parsing logic is inlined here into the writer.
It seems like this should be a responsibility of the reader, such that it would present and interface for iterating over each binary ID semantically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135929