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

Add more extension as described in Issue #4 #18

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

jriyyya
Copy link

@jriyyya jriyyya commented Apr 9, 2024

This PR aims to continue the remaining work from commit 7c1eae7 of issue #4 converting all uses of haveZdinx(), haveZhinx(), haveZfinx() to extension("Zdinx"), extension("Zhinx"), extension("Zfinx") respectively to facilitate parsing.

@ThinkOpenly
Copy link
Owner

Thanks, @jriyyya! The code looks fine to me. It compiles and produces the same output as before, as expected. You've marked this PR as "draft", so I will hold off merging it for now.

Really minor comments:

  • The commit subject lines are similar, but do differ in whether they include "function extension" or "extension function".
  • You use "function extension" and "haveZinx()" in the subjects. Both are functions, so I'd prefer seeing the same convention for both. Perhaps "Utilize extension() instead of haveZinx()".
  • If you wanted to make sure future observers understood what you were doing and why, you could add a body to the commit message with explanation. Even something simple like "Continue the remaining work from commit 7c1eae7 in converting all uses of have<extension-name>() to extension("<extension-name") to facilitate parsing." or something like that.

With or without any of those suggestions implemented (as I said it's fine as-is), remove the "draft" marking when you are ready for me to merge this!

@jriyyya jriyyya changed the title [WIP] Add more extension as described in Issue #5 Add more extension as described in Issue #4 Apr 9, 2024
@jriyyya
Copy link
Author

jriyyya commented Apr 9, 2024

Thanks for the Review. I was actually planning to continue this PR and further add extension function for D and F. We can also open another PR for the same, So removing the draft here.

Sorry about the commit message. Should Squash these three commits into to a single commit as "Utilize extension() instead of haveZdinx(), haveZfinx(), haveZhinx" ?

I thought of adding a description when I would have removed the draft tag, I have added it now. I will keep in mind to add it beforehand next time 😅.

@jriyyya jriyyya marked this pull request as ready for review April 9, 2024 14:45
@ThinkOpenly
Copy link
Owner

grepping through the code, I see a need to make similar changes for haveSupMode, haveUsrMode, haveNExt, haveZmmul, and haveZkr (at least).

(A good test might be to comment out all of the have* functions in model/riscv_sys_regs.sail, and see what errors are produced when building. I have not done this.)

For now, what you have posted is great. I'll merge this, and you can continue with those, at your option, when you are able. Thanks, again!

@ThinkOpenly ThinkOpenly merged commit 59b3b5a into ThinkOpenly:JSON Apr 9, 2024
1 check passed
@jriyyya
Copy link
Author

jriyyya commented Apr 10, 2024

(A good test might be to comment out all of the have* functions in model/riscv_sys_regs.sail, and see what errors are produced when building. I have not done this.)

Quite a nice idea, I will give it a try!

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.

2 participants