[Lldb-commits] [PATCH] D122603: [wip][intelpt] Refactor timestamps out of `IntelPTInstruction`

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

I'm proposing a new interface for the TscRange. Let me know if you have questions



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:95
+  m_instructions.emplace_back(insn);
+  if (__last_tsc != tsc) {
+    m_instruction_timestamps.emplace(m_instructions.size() - 1, tsc);
----------------
we need to update it because of the optional


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:126-127
+
+  /// Append a timestamp at the index of the last instruction.
+  void AppendInstruction(const pt_insn& instruction, uint64_t timestamp);
+
----------------
Let's improve the documentation and let's use the word TSC more ubiquitously 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:144
+
+  // Range of timestamp iterators for a given index
+  struct TscRange {
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:145-153
+  struct TscRange {
+    uint64_t tsc;
+    size_t start_index;
+    size_t end_index;
+    std::map<size_t, uint64_t>::const_iterator next;
+    std::map<size_t, uint64_t>::const_iterator prev;
+
----------------
Let's add more logic to this object so that it handles as much as it can and we reduce the logic that was added to DecodedThread. We also don't need two iterators, just one is enough.  Don't forget to add documentation to these new methods.


Given this new definition for TscRange. The only method we need to add in DecodedThread is CalculateTscRange(size_t insn_index), and mention the documentation that this operation is O(logn) and should be used sparingly.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:146-148
+    uint64_t tsc;
+    size_t start_index;
+    size_t end_index;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:156-163
+  /// Get timestamp of an instruction by its index.
+  llvm::Optional<struct TscRange> GetTSCRange(size_t insn_index) const;
+
+  /// Get timestamp of an instruction by its index.
+  llvm::Optional<struct TscRange> GetNextTSCRange(const TscRange& range) const;
+  
+  /// Get timestamp of an instruction by its index.
----------------
Let's improve the documentation and also get rid of the `struct` keyword in return types. That's old style C


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:196-202
   lldb::ThreadSP m_thread_sp;
   std::vector<IntelPTInstruction> m_instructions;
+  std::map<size_t, uint64_t> m_instruction_timestamps;
   llvm::DenseMap<uint64_t, std::string> m_errors;
   llvm::Optional<size_t> m_raw_trace_size;
+
+  uint64_t __last_tsc;
----------------
don't start variable names with __, as may people think that those variables should be discarded. Let's just give it a proper name. Let's also use an Optional and let's add documentation to all the variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603