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

refactor mstatus handler function #652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KotorinMinami
Copy link
Contributor

As mentioned in #639, and there are also some legitimacy issues in legalize_mstatus, so this PR is intended to help make corrections

Copy link

github-actions bot commented Dec 20, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 8633212. ± Comparison against base commit ff4077c.

♻️ This comment has been updated with latest results.

@KotorinMinami KotorinMinami changed the title reflact mstatus handler function refactor mstatus handler function Dec 26, 2024
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_vext_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_control.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
@KotorinMinami KotorinMinami force-pushed the reflect_mstatus branch 2 times, most recently from 8c3ba10 to 0953d93 Compare January 9, 2025 09:32
@KotorinMinami
Copy link
Contributor Author

@Timmmm Is there anything else that needs improvement in my code?

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

LGTM - would be nice to get a second opinion though. Any takers @arichardson @jordancarlin etc?

Copy link
Collaborator

@arichardson arichardson 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 like an improvement to me. Nit: not sure we need to add all the future fields in this PR but I don't mind keeping them here.

@Timmmm Timmmm added the will be merged Scheduled to be merged in a few days if nobody objects label Jan 9, 2025
Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

Minor nit in a comment but otherwise LGTM

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will be merged Scheduled to be merged in a few days if nobody objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants