[PATCH] D147287: [InstrProf] Temporal Profiling

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

Looks good overall, added a few comments which are mostly cosmetic.



================
Comment at: compiler-rt/include/profile/InstrProfData.inc:670
 #define GET_VERSION(V) ((V) & ~VARIANT_MASKS_ALL)
+#define VARIANT_MASK_TEMPORAL_PROF (0x1ULL << 55)
 #define VARIANT_MASK_IR_PROF (0x1ULL << 56)
----------------
If we used the 63rd bit then we wouldn't have to update the mask. Is there reason why we can't do so?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:535
   virtual bool hasMemoryProfile() const = 0;
+  virtual bool isTemporalProfile() const = 0;
   virtual InstrProfKind getProfileKind() const = 0;
----------------
I think in the common case the indexed format will contain instrumentation profiles and TemporalProfiles. If so, then naming this `hasTemporalProfile` would be nice. 


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:270
       ProfileKind &= ~InstrProfKind::FunctionEntryInstrumentation;
-    else
+    else if (Str.equals_insensitive("traces")) {
+      ProfileKind |= InstrProfKind::TemporalProfile;
----------------
Prefer a more descriptive text here: "temporal_profile" or "function_traces". Also see other comment about using a consistent naming scheme.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:1141
+    const auto *PtrEnd = (const unsigned char *)DataBuffer->getBufferEnd();
+    if (Ptr + 2 * sizeof(uint64_t) > PtrEnd)
+      return error(instrprof_error::truncated);
----------------
Add a comment about the expected information to be read, i.e. NumTraces and TraceStreamSize?


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:303
+    // Otherwise, replace a random trace in the stream.
+    std::uniform_int_distribution<uint64_t> Distribution(0, TraceStreamSize);
+    uint64_t RandomIndex = Distribution(RNG);
----------------
This feels like it would lead to non-deterministic builds where the raw profile is an input to the build. Is the seed deterministic?


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:341
+  llvm::shuffle(SrcTraces.begin(), SrcTraces.end(), RNG);
+  for (auto [Index, Trace] : llvm::zip(IndicesToReplace, SrcTraces))
+    Traces[Index] = std::move(Trace);
----------------
Can we use auto& to avoid a copy for Trace?


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:440
   Header.BinaryIdOffset = 0;
+  Header.FunctionTracesOffset = 0;
   int N = sizeof(IndexedInstrProf::Header) / sizeof(uint64_t);
----------------
There seem to be a few different phrases referring to the same functionality, e.g. `TemporalProfile`, `FunctionTraces` and just `Trace` in other places. It would be nice to have a single prefix for all the var and function names to be able to quickly navigate the code. What do you think?


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:233
+  uint64_t MaxTraceLength = 2;
+  InstrProfWriter Writer(false, TraceReservoirSize, MaxTraceLength);
+  ASSERT_THAT_ERROR(Writer.mergeProfileKind(InstrProfKind::TemporalProfile),
----------------
Add a comment for the param set to false, I think it should be `/*Sparse=*/=false`. Here and below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147287