riscv-software-src / riscv-perf-model

Example RISC-V Out-of-Order/Superscalar Processor Performance Core and MSS Model
Apache License 2.0
117 stars 43 forks source link

Dromajo STF Trace Support Improvements #130

Open kathlenemagnus opened 7 months ago

kathlenemagnus commented 7 months ago

I have been reviewing the STF trace generation support in Dromajo and have some recommendations to improve the implementation.

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.

danbone commented 7 months ago

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

jeffnye-gh commented 7 months ago

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.

kathlenemagnus commented 7 months ago

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.

jeffnye-gh commented 7 months ago

I was asking if new behavior was intended with --stf-tracepoint since the existing trace macros are enabled with --stf_trace and disabled without.

kathlenemagnus commented 7 months ago

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.

kathlenemagnus commented 7 months ago

@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.

jeffnye-gh commented 7 months ago

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 commented 7 months ago

@jeffnye-gh I'll do that. Thanks!

klingaard commented 4 months ago

@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.

kathlenemagnus commented 4 months ago

@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 commented 4 months ago

Thank you @kathlenemagnus!

jeffnye-gh commented 4 months ago

@klingaard @kathlenemagnus

The SHA with KM's changes is

commit ff13255

aka ff13255a50bd1b5e7597f3bf2ceaf24b782e8b72