[PATCH] D147287: [InstrProf] Temporal Profiling

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfiling.c:27
+void INSTR_PROF_PROFILE_SET_TIMESTAMP(uint64_t *Probe) {
+  if (*Probe == 0 || *Probe == (uint64_t)-1)
+    *Probe = __llvm_profile_global_timestamp++;
----------------
davidxl wrote:
> what is -1 value reserved for?
For counts instrumentation the `__llvm_prf_cnts` section is initialized to zero while for coverage instrumetation the section is initialized to all ones. So both `0` and `-1` are uninitialized values. See https://github.com/llvm/llvm-project/blob/a78997e0aa89250a193ff036e7c6e71562e03222/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L911-L926


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:644
+      if (hasSingleByteCoverage())
+        I += 7;
+      continue;
----------------
davidxl wrote:
> what is this increment?
I added a clarifying comment. Hopefully it makes sense :)


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:311
+
+void InstrProfWriter::addFunctionTraces(SmallVector<InstrProfTraceTy> SrcTraces,
+                                        uint64_t SrcTraceStreamSize) {
----------------
davidxl wrote:
> Is it better to do  deeper merge, e.g.   b -c -d  can be merged with a-b-c-d-e,   and a-b and be merged with b-c.   A trace count can also be added for each merged trace to show the weight.
That's an interesting idea, but I think this will lose information. If `b-c-d` is merged with `a-b-c-d-e` then we no longer remember that `b` was the first function in a trace. This is important because functions called early in a trace are important during startup.

Also, in practice I would guess most traces would differ in small sections scattered throughout the trace, making this type of merging less useful. We could probably "compress" these traces by storing common runs of functions, but this is overkill IMO. With `TraceReservoirSize = 100` and `MaxTraceLength = 10000` the max overhead is 8Mb I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147287