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

Make mstatus[VS] dirty when write to vstart #623

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rez5427
Copy link
Contributor

@rez5427 rez5427 commented Nov 26, 2024

fix #569

Copy link

github-actions bot commented Nov 26, 2024

Test Results

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

Results for commit e6a8923. ± Comparison against base commit 07fa23e.

♻️ This comment has been updated with latest results.

@jrtc27
Copy link
Collaborator

jrtc27 commented Nov 26, 2024

misa[V] == 0b1 implies sys_enable_vext(), and the commit message is extremely devoid of any detail. Per CONTRIBUTING.md:

Each commit should be self-contained, provide a meaningful short summary in the subject and, if not clear from the subject alone, provide more detail in the commit description.

But that's also just a common expectation for software engineering in general, it shouldn't need to be spelled out.

@rez5427 rez5427 changed the title Add vext runtime support Make misa[V] useful in runtime Nov 26, 2024
@Timmmm
Copy link
Collaborator

Timmmm commented Nov 26, 2024

I'm not sure this is correct either. I think we can get rid of dirty_v_context_if_present() and just call dirty_v_context(). Currently it is only called from ext_write_vcsr and that should only happen if V is enabled.

Emphasis on should because currently the is_CSR_defineds are missing extensionEnabled(Ext_V):

function clause is_CSR_defined (0x009) = true

I opened #624 to track that.

I believe dirty_fd_context_if_present() exists because of Zfinx... but I don't think that's correct either. When you write to fcsr it should dirty mstatus[FS] even with Zfinx but that currently doesn't happen. I opened #625 to track that.

Also this doesn't completely fix #569 - that issue also says that there are missing calls to dirty_v_context....

@rez5427
Copy link
Contributor Author

rez5427 commented Nov 26, 2024

Maybe this is good?

@Timmmm
Copy link
Collaborator

Timmmm commented Nov 26, 2024

This looks a lot better. There are still some more places where the context needs to be dirtied, e.g. search for vcsr[vxsat] = 0b1; (I would extract it into a set_vcsr_vxsat() function or something). There's also a gazillion places where vstart is written directly. I would make a set_vstart() too.

Doesn't have to be in this PR though. In fact maybe better if it isn't...

@rez5427 rez5427 changed the title Make misa[V] useful in runtime Make mstatus[VS] dirty when write to vstart Nov 27, 2024
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. I'll squash and fix the commit message when merging, which I will do in a few days if nobody objects.

@Timmmm Timmmm added the will be merged Scheduled to be merged in a few days if nobody objects label Nov 27, 2024
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.

Vector Status flag mstatus[VS] is not updated correctly
6 participants