Expose per-step pre_step/post_step callbacks #1395
Conversation
pmundkur
left a comment
pmundkur
left a comment
There was a problem hiding this comment.
I'm curious what use case you had in mind for these callbacks. Is it worth generalizing the pre_step callback to include rvfi->pre_step?
The main use cases I had in mind for these per-step callbacks are: |
Timmmm
changed the title
Timmmm
left a comment
Timmmm
left a comment
There was a problem hiding this comment.
Sorry for the slow review - this looks great. Just a few minor renamings to match the existing name style.
I thought that too, but RVFI has special flow control stuff that's a bit awkward. I think we should probably just special-case it for now and think of something better in a future PR. As for use cases, I need this exact thing for binary output support. You can collect information for a step in the callbacks then commit it in @ibvqeibob let me know if you would be offended if I make the renaming changes - I suddenly have an immediate need for JSON output and it will be easier with this PR. |
Co-authored-by: Tim Hutt <[email protected]> Signed-off-by: uestc_yd <[email protected]>
Co-authored-by: Tim Hutt <[email protected]> Signed-off-by: uestc_yd <[email protected]>
Co-authored-by: Tim Hutt <[email protected]> Signed-off-by: uestc_yd <[email protected]>
Co-authored-by: Tim Hutt <[email protected]> Signed-off-by: uestc_yd <[email protected]>
Co-authored-by: Tim Hutt <[email protected]> Signed-off-by: uestc_yd <[email protected]>
Thanks a lot for the review and for pushing the renaming changes – they look great to me. |
|
Thanks, there were a few more renamings to do and some unused variable warnings which I've fixed. LGTM now. |
| if (!is_waiting) { | ||
| if (config_print_step) { | ||
| fprintf(trace_log, "\n"); | ||
| } | ||
| step_no++; | ||
| insn_cnt++; | ||
| total_insns++; | ||
| } | ||
|
|
||
| if (do_show_times && (total_insns & 0xfffff) == 0) { | ||
| uint64_t start_us = 1000000 * ((uint64_t)interval_start.tv_sec) | ||
| + ((uint64_t)interval_start.tv_usec); | ||
| if (gettimeofday(&interval_start, nullptr) < 0) { | ||
| fprintf(stderr, "Cannot gettimeofday: %s\n", strerror(errno)); | ||
| exit(EXIT_FAILURE); | ||
| } | ||
| uint64_t end_us = 1000000 * ((uint64_t)interval_start.tv_sec) | ||
| + ((uint64_t)interval_start.tv_usec); | ||
| fprintf(stdout, "kips: %" PRIu64 "\n", | ||
| ((uint64_t)1000) * 0x100000 / (end_us - start_us)); | ||
| } |
There was a problem hiding this comment.
After this is merged I think we can move almost all of this stuff into the log callback handler.
jordancarlin
left a comment
jordancarlin
left a comment
There was a problem hiding this comment.
One minor nit, but LGTM.
| void log_callbacks::pre_step_callback(bool is_waiting) | ||
| { | ||
| // TODO: per-step logging or statistics | ||
| (void)is_waiting; | ||
| } | ||
|
|
||
| void log_callbacks::post_step_callback(bool is_waiting) | ||
| { | ||
| // TODO: post-step logging or statistics | ||
| (void)is_waiting; | ||
| } |
There was a problem hiding this comment.
No need to include the empty implementations here since we added the [[maybe_unused]] annotations.
5 participants
Add pre/post-step callbacks. These are currently unused but will be used in future by log/trace outputs and maybe RVFI.
callbacks_ifwith{pre,post}_step_callback(bool is_waiting)default no-op virtual methods.call_{pre,post}_step_callbackshelpers inriscv_callbacks.{h,cpp}to fan out the new callbacks to all registered handlers.{pre,post}_step_callbackaround eachztry_stepinrun_sail(), passing the currentis_waitingstate.{pre,post}_step_callbackinlog_callbacksas no-op placeholders with TODOs for future per-step logging/statistics.Related to: #1353