vsk added a comment. Hey @jj10306, thanks for working on this.
@wallace @clayborg -- It seems like there are a ton of logic changes introduced in this patch that are tested at too coarse a level. I'm not confident in the changes being made here. For example, there's a bunch of subtle work being done in `BasicSuperBlockMerge`, but only ~one opaque high-level check that attempts to validate any of the logic: `self.assertTrue(num_units_by_layer[1] == 383)`. Where does 383 come from? How do we know that that's the right result? If there's a bug in `BasicSuperBlockMerge`, would the regression test just look like changing the magic 383 number? I'm not really comfortable with this being checked in, and would suggest a revert until some more granular unit tests can be added to the patch. (Also paging @JDevlieghere in case he has thoughts on the testing.) ================ Comment at: lldb/docs/htr.rst:27 +.. image:: media/htr-example.png + +Passes ---------------- Would be helpful to describe a brief feature roadmap here, explaining what debugger functionality can be built on top of this HTR concept. ================ Comment at: lldb/source/Plugins/TraceExporter/common/TraceHTR.h:38 + size_t num_instructions, + llvm::DenseMap<ConstString, size_t> &func_calls) + : m_first_instruction_load_address(first_instruction_load_address), ---------------- Why not move 'func_calls' instead of copying it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105741/new/ https://reviews.llvm.org/D105741 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
