[PATCH] D147287: [InstrProf] Temporal Profiling

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
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)
----------------
snehasish wrote:
> If we used the 63rd bit then we wouldn't have to update the mask. Is there reason why we can't do so?
Oh my mistake, we can in fact use the 63rd bit here. I think I was anticipating this bit being used in my earlier revisions. Thanks for catching!


================
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);
----------------
snehasish wrote:
> This feels like it would lead to non-deterministic builds where the raw profile is an input to the build. Is the seed deterministic?
I've added `std::mt19937 RNG` to `InstrProfWriter` which has a constant default seed which should make this deterministic.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:440
   Header.BinaryIdOffset = 0;
+  Header.FunctionTracesOffset = 0;
   int N = sizeof(IndexedInstrProf::Header) / sizeof(uint64_t);
----------------
snehasish wrote:
> 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?
I see what you mean. I'm using `FunctionTrace` and `Trace` interchangeably and they aren't very descriptive. What do you think of the `TP` (Temporal Profiling) prefix? So I would use `TPTrace` in code and `:tp_traces` for the text format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147287