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

Remove N extension #584

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Remove N extension #584

merged 1 commit into from
Oct 16, 2024

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Oct 9, 2024

This extension was never ratified, has been removed from the ISA manual and apparently would need significant changes if it were to ever come back.

Therefore it makes sense to remove it from the model in order to simplify the code.

This was written by @KotorinMinami in #531, I then made some minor formatting changes and some refactoring of the interrupt handling in this commit, and finally rebased it.

Copy link

github-actions bot commented Oct 9, 2024

Test Results

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

Results for commit eac9139. ± Comparison against base commit 2b65a0d.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Alasdair Alasdair left a comment

Choose a reason for hiding this comment

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

This was one of the two awkward extensions that made my modularisation attempt tricky, so I'm not too sad that it's gone

@Timmmm
Copy link
Collaborator Author

Timmmm commented Oct 9, 2024

Yeah it's making writing CLIC a pain too, e.g. it's difficult to make a function get_xcause(p : Privilege) -> xlenbits due to the compilation order and I thought about making it a scattered function but I decided getting this merged would be better.

Copy link
Contributor

@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.

I was looking at moving the CSRs to actually be scattered, and this was causing compilation order issues for that too. Will be great to get rid of it since it touches so many parts of the model.

@KotorinMinami
Copy link
Contributor

@Timmmm There's absolutely nothing wrong with anything you've done. On the contrary, I'm quite pleased and grateful that you've managed to help resolve the conflicts when merge into master. Similarly, I do hope this PR can be merged and make a valuable contribution to our gold model.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Oct 11, 2024

@billmcspadden-riscv any objection to merging this?

@Timmmm
Copy link
Collaborator Author

Timmmm commented Oct 15, 2024

I think we've got good agreement on this anyway and it will make Alasdair and Jordan's lives easier so I'll merge it on Thursday if nobody objects before then.

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.

LGTM.

However, I would not be surprised if this extension does not raise its head again.
It's been kicked around for at least 5 years and at one point, someone important swore that it would be ratified "real soon now".

@billmcspadden-riscv
Copy link
Collaborator

billmcspadden-riscv commented Oct 15, 2024 via email

This extension was never ratified, has been removed from the ISA manual and apparently would need significant changes if it were to ever come back.

Therefore it makes sense to remove it from the model in order to simplify the code.
@Timmmm Timmmm merged commit 5cea520 into riscv:master Oct 16, 2024
2 checks passed
@Timmmm Timmmm deleted the user/timh/remove_n branch October 28, 2024 13:09
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