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

Move extension scattered enum to single file #634

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Dec 6, 2024

Having this distributed through the code can lead to slightly awkward dependency order issues. We need to keep it scattered though so e.g. CHERI can add its extensions.

Fixes #633

Copy link

github-actions bot commented Dec 6, 2024

Test Results

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

Results for commit 0f89f4e. ± Comparison against base commit b102d07.

♻️ This comment has been updated with latest results.

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 6, 2024

This doesn’t work for sail-cheri-riscv.

@Alasdair
Copy link
Collaborator

Alasdair commented Dec 6, 2024

I think the enumeration should probably remain scattered, but it seems to me there are two cases:

  • Standard extensions that are declared up-front, and that appear in misa. We could declare all those standard single letter extensions in one place

  • Other extensions that don't appear in misa, e.g. Xcheri

@Timmmm
Copy link
Collaborator Author

Timmmm commented Dec 6, 2024

Hmm good point about CHERI. Unfortunately it isn't just misa. There's also things like menvcfg/mseccfg which have circular dependencies.

I guess technically the right thing to do would be to make legalize_menvcfg etc. scattered functions, but that just seems like overkill.

How about we keep it scattered but still move them all into one file?

@arichardson
Copy link
Collaborator

This doesn’t work for sail-cheri-riscv.

True, but IMO sail-cheri-riscv should change to just be a diff on top of the base model. That would make upstreaming a lot easier as well.

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 6, 2024

This doesn’t work for sail-cheri-riscv.

True, but IMO sail-cheri-riscv should change to just be a diff on top of the base model. That would make upstreaming a lot easier as well.

It should, but it won’t do so immediately, and the model has been designed to support the current approach, so we do need to uphold that.

@Timmmm Timmmm force-pushed the user/timh/extensions_enum branch from 21a44d6 to 25c5b01 Compare December 16, 2024 14:03
@Timmmm Timmmm changed the title Move extension scattered enum to non-scattered enum Move extension scattered enum to single file Dec 16, 2024
@Timmmm Timmmm force-pushed the user/timh/extensions_enum branch from 25c5b01 to 859a49c Compare December 16, 2024 14:30
@Timmmm Timmmm force-pushed the user/timh/extensions_enum branch from 859a49c to 8a70125 Compare December 18, 2024 16:58
@Timmmm Timmmm requested a review from jordancarlin December 19, 2024 22:20
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.

LGTM now that comments are consistently added. Keeping it as a scattered enum should avoid the issues with CHERI while giving us the benefit of simpler file ordering.

I wonder if it might be nicer to order the extensions by canonical order instead of alphabetically so that extensions like Zfinx and Zdinx are next to each other, but it's also fine as it is. If you wanted to use the same ordering that I put in the README that would work.

Having this distributed through the code can lead to slightly awkward dependency order issues. We need to keep it scattered though so e.g. CHERI can add its extensions.
@Timmmm Timmmm force-pushed the user/timh/extensions_enum branch from 8a70125 to 0f89f4e Compare December 20, 2024 14:27
@Timmmm
Copy link
Collaborator Author

Timmmm commented Dec 20, 2024

Sorted according to the canonical ordering. Which actually doesn't make any sense for Zhinh since it's supposed to be related to Hypervisor. Eh whatever.

@jordancarlin
Copy link
Collaborator

Hmm. Really should've been named Zfhinx or something. I'd say this is still an improvement though.

@Timmmm Timmmm added the will be merged Scheduled to be merged in a few days if nobody objects label Dec 20, 2024
@Timmmm Timmmm merged commit c855281 into riscv:master Dec 30, 2024
2 checks passed
@Timmmm Timmmm deleted the user/timh/extensions_enum branch January 2, 2025 11:49
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.

Make extension enum not scattered
5 participants