jj10306 requested changes to this revision.
jj10306 added inline comments.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288
+ decoded_thread.NotifyTsc(execution.thread_execution.GetLowestKnownTSC());
decoded_thread.NotifyCPU(execution.thread_execution.cpu_id);
----------------
if we have both a start and end TSC, shouldn't we emit the start here (like
you're doing) and then after decoding all the instructions we should also emit
the the end TSC?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:100-103
+ // This is the last TSC range, so we have to extrapolate. In this case, we
+ // assume that each instruction took one TSC, which is what an instruction
+ // would take if no parallelism is achieved and the frequency multiplier
+ // is 1.
----------------
Is this correct?
I don't see an issue with choosing a 1 TSC per instruction as a heuristic in
this case, but we should be cautious not to explain this heuristic as being
tied to any "expected" performance of the hardware because there are way too
many variables that influence IPC like differences in workload,
microarchitecture (speculative execution/branch prediction implementation,
vector units, super scalar, etc), etc not to mention the difference b/t TSC and
CPU cycles (like you mentioned).
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:105
+ return m_tsc_conversion->ToNanosFloat(lowest_nanos,
+ range->tsc + items_since_last_tsc);
+ }
----------------
Won't our interpolation calculation be messed up since `items_since_last_tsc`
will be counting all trace errors, events, etc and not just the
instructions/events that we want to assign a timestamp to and thus should be
part of this calculation?
I'm thinking of a case like below :
i - instruction, e - error
`tsc, cpu_change, i, e, e, e, tsc`
in this case the items_since last_tsc would be 5 even though there is only 1
instruction.
i guess this brings up a more general question around how timestamps should be
assigned. For example, in this case I see a couple different options:
1. What we are doing now where we assign all events timestamps
Pro: easy implementation
Con: visualization may be weird bc timestamps are being influenced by events
(ie errors) that users of the visualization don't care or know about
2. Ignore errors but treat all instructions and events the same (ie
instructions and events get timestamps)
3. Ignore errors and treat instructions and events differently (ie instructions
are the only things that contribute to `items_since_last_tsc`) and then events
are simply assigned the same timestamp as an instruction
We can discuss this in more depth offline.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130054/new/
https://reviews.llvm.org/D130054
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
