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

Setting F=0 and D=1 should result in both F and D being cleared #301

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

ahadali-10x
Copy link

Changed

  • Updated legalize_misa function in the riscv_sys_regs.sail in the model/ directory to add condition by setting F=0 and D=1 should clear both F and D.

Issue Fixed

@ahadali-10x ahadali-10x force-pushed the F/D_ext_bug branch 2 times, most recently from c56fe8e to 7123a7b Compare August 30, 2023 14:53
@github-actions
Copy link

github-actions bot commented Aug 30, 2023

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit e9ee198. ± Comparison against base commit 24e3e68.

♻️ This comment has been updated with latest results.

@Timmmm
Copy link
Collaborator

Timmmm commented Sep 18, 2023

This looks correct to me.

@ahadali-10x
Copy link
Author

@billmcspadden-riscv @scottj97 Kindly review the changes.

@billmcspadden-riscv
Copy link
Collaborator

@ahadali-10x : please add the following reference to your commit message:

"Per section 3.1.1 of the Privileged Spec ("Machine ISA Register misa"), ...."

Copy link
Collaborator

@billmcspadden-riscv billmcspadden-riscv left a 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.

@ahadali-10x
Copy link
Author

@billmcspadden-riscv This PR can be merged now.

@billmcspadden-riscv billmcspadden-riscv merged commit dbea780 into riscv:master Sep 26, 2023
2 checks passed
@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 26, 2023

Commit message was poorly formatted, subject was too long and contained all the detail, rather than a short subject with a detailed body.

@Alasdair
Copy link
Collaborator

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?

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 26, 2023

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

@Timmmm
Copy link
Collaborator

Timmmm commented Sep 26, 2023

Ah yeah I think Jessica is right - we might be looking at different things. The PR title is fine:

Setting F=0 and D=1 should result in both F and D being cleared

(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:

Per section 3.1.1 of the Privileged Spec (Machine ISA Register misa): F/D both should be disabled if F=0

Probably should have been something like

Clear misa F and D bits when F=0 and D=1

With the spec reference in the body.

Relatively minor issue IMO though.

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.

6 participants