Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc fixes #2086

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Misc fixes #2086

merged 3 commits into from
Feb 5, 2024

Conversation

GregAC
Copy link
Collaborator

@GregAC GregAC commented Sep 13, 2023

A collection of small fixes/changes produced whilst I was running coremark to get the latest Ibex performance numbers across different configurations.

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, with a slight style question about a README.

@@ -11,9 +11,12 @@ All of these benchmarks run on Simple System. A verilator simulation suitable
for running them can be built with:

```
fusesoc --cores-root=. run --target=sim --setup --build lowrisc:ibex:ibex_simple_system --RV32E=0 --RV32M=ibex_pkg::RV32MFast
fusesoc --cores-root=. run --target=sim --setup --build lowrisc:ibex:ibex_simple_system `./util/ibex_config.py [ibex_config] fusesoc_opts`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe it's cleaner to give an example config here? (That way a reader can copy-paste from the doc to their terminal!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I switched this to specify 'maxperf' as the config so it can be copy/pasted. I didn't want to include explicit flags as the idea is you should just use the ibex_config.py script to generate them for you based upon a config name.

Some may wish to specify manually but I'd consider that a more advanced usage and these instructions are targetted for people just trying to get Ibex doing a few things, such as reproduce benchmark results against supported configs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rswarbrick does that address your question?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Yep, that sounds reasonable to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aargh! Sorry, I forgot! I'd suggest picking a config here, and writing something below about how to change it. It seems importantant (to me) that a reader can just copy/paste something into the terminal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example command line can be directly copy/pasted and used.

Though the wording below maybe didn't make this obvious, I've updated it to make it explicitly (hopefully!).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh, sorry. I was looking at the thing from GitHub at the top of this discussion, which is what I originally commented on. What you've got looks good to me.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, except the change that Harry requested.

@GregAC
Copy link
Collaborator Author

GregAC commented Jan 31, 2024

My apologies for the rather delayed response! Thanks for reviewing @hcallahan-lowrisc @rswarbrick @marnovandermaas would you mind taking another look?

@rswarbrick
Copy link
Contributor

I still have my question about the example config, but this looks reasonable otherwise.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me.

@GregAC GregAC added this pull request to the merge queue Feb 5, 2024
Merged via the queue into lowRISC:master with commit 1774cbb Feb 5, 2024
11 checks passed
@GregAC GregAC deleted the misc_fixes branch February 6, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants