riscv-perf-model
riscv-perf-model copied to clipboard
riscv-software-src
Dromajo STF Trace Support Improvements
I have been reviewing the STF trace generation support in Dromajo and have some recommendations to improve the implementation.
- Add register read and write records
- Add memory read and write records
- Refactor parameters to control enablement of tracepoint and privilege mode checks
- Better handling of "no trace generated" scenarios (no trace created + a warning if no instructions are traced)
These are the current STF trace parameters in Dromajo:
--stf_trace <filename> Dump an STF trace to the given file"
--stf_no_priv_check Turn off the privledge check in STF generation"
I would like to propose removing --stf_no_priv_check and adding these parameters:
--stf-tracepoint Enable tracepoint detection for STF trace generation"
--stf-priv-modes <MHSU|HSU|SU|U> Specify which privilege modes to include in STF trace generation"
These parameters would allow the user to specify whether tracepoints should be used to start/stop trace generation and specify which privilege modes to include in the trace. All conditions specified must be met for tracing to be enabled. These new options would also allow for noncontiguous traces, which is well handled by the STF library.
Sorry could you expand on what you mean by:
These parameters would allow the user to specify whether tracepoints should be used to start/stop trace generation
Sounds good to me, my comments:
I like the generality of replacing no priv check.
I also would like to know more about --stf-tracepoint
It's pretty quick for me to run our process against a PR/draft PR when it's available.
Tracepoints are just hint instructions with a special meaning.
xor x0, x0, x0 // tracepoint to start tracing
xor x0, x1, x1 // tracepoint to stop tracing
In the current Dromajo STF tracing implementation, tracepoints are required to start and stop tracing. Adding this new option would make them optional.
I was asking if new behavior was intended with --stf-tracepoint since the existing trace macros are enabled with --stf_trace and disabled without.
Yes, so --stf_trace would be required to enable tracing and --stf-tracepoint would enable tracepoint detection. By default, tracepoint detection would be disabled and the --stf-tracepoint parameter would be ignored if --stf_trace is not set.
BTW I intended to follow the style of the existing parameters so --stf-tracepoint will actually be --stf_tracepoint.
@jeffnye-gh how would you like me to push my changes to the Condor Dromajo repo? I don't have write permissions so I can't push my branch.
Does it work if you create a pull request from your fork of the Condor Dromajo repo?
The method we use for riscv-perf-model, where we also do not have write permission, was explained by knute
"... What folks do is fork the repo into their own private space, create branches and push to those branches to their private forks. To push your private branch to the main repo, you start by creating a new pull request (I believe in your private fork) and set pull downs to the proper source/destination locations."
On Thu, Dec 21, 2023 at 3:19 PM Kathlene Magnus @.***> wrote:
@jeffnye-gh https://github.com/jeffnye-gh how would you like me to push my changes to the Condor Dromajo repo? I don't have write permissions so I can't push my branch.
— Reply to this email directly, view it on GitHub https://github.com/riscv-software-src/riscv-perf-model/issues/130#issuecomment-1866941788, or unsubscribe https://github.com/notifications/unsubscribe-auth/A25MMC4IWVJ66SGHJWXV6Y3YKSRUXAVCNFSM6AAAAABAQKH6H6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRWHE2DCNZYHA . You are receiving this because you were mentioned.Message ID: @.***>
@kathlenehurt-sifive, when you're done with this issue, pass it over to me (I'll hijack it) and I'll move Olympia (via documentation) to illustrate the new flow.
@klingaard this is good to go. Olympia should point to the master branch of Condor's Dromajo fork: https://github.com/Condor-Performance-Modeling/dromajo/tree/master
The STF lib version also needs to be moved to the latest to pull in a required fix for the PC record.
@klingaard @kathlenemagnus
The SHA with KM's changes is
commit ff13255
aka ff13255a50bd1b5e7597f3bf2ceaf24b782e8b72