-
Notifications
You must be signed in to change notification settings - Fork 167
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
Setting F=0 and D=1 should result in both F and D being cleared #301
Conversation
c56fe8e
to
7123a7b
Compare
This looks correct to me. |
@billmcspadden-riscv @scottj97 Kindly review the changes. |
@ahadali-10x : please add the following reference to your commit message: "Per section 3.1.1 of the Privileged Spec ("Machine ISA Register misa"), ...." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me.
… F/D both should be disabled if F=0
7123a7b
to
e9ee198
Compare
@billmcspadden-riscv This PR can be merged now. |
Commit message was poorly formatted, subject was too long and contained all the detail, rather than a short subject with a detailed body. |
Commit message looks fine to me. Says precisely what was fixed. Do we have precise rules on what a commit message should be in the style guide? |
No, because it’s standard practice for any well-managed open source project. You can tell it’s not a good commit message because GitHub cuts the subject off; were it correctly written the subject would be short enough to not be cut off. Git’s own manpage for the commit command documents this best practice (though 50 characters is a bit aggressive, IIRC GitHub caps at 80:) https://manpages.debian.org/bookworm/git-man/git-commit.1.en.html#DISCUSSION |
Ah yeah I think Jessica is right - we might be looking at different things. The PR title is fine:
(except that it's a description of the bug, not the fix) The commit message is very long and not in the normal commit message style:
Probably should have been something like
With the spec reference in the body. Relatively minor issue IMO though. |
Changed
legalize_misa
function in theriscv_sys_regs.sail
in themodel/
directory to add condition by setting F=0 and D=1 should clear both F and D.Issue Fixed