-
Notifications
You must be signed in to change notification settings - Fork 228
Refactor vector extension configuration. #1163
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the feedback! I’ve made the changes and updated the |
I've updated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependencies for the vector bit manipulation and cryptography extensions need to change too:
The Zvknhb and Zvbc Vector Crypto Extensions --and accordingly the composite extensions Zvkn, Zvknc, Zvkng, and Zvksc-- depend on Zve64x.
All of the other Vector Crypto Extensions depend on Zve32x.
Note: If Zve32x is supported then Zvkb or Zvbb provide support for EEW of 8, 16, and 32. If Zve64x is supported then Zvkb or Zvbb also add support for EEW 64.
I’ve updated the As for:
I’ll handle those SEW constraints in the instruction-level changes. |
Thanks for pointing that out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting these all added! LGTM, but I think we should wait until after the upcoming release before merging to avoid confusion because #1170 probably won’t be ready.
@wwwwwwOwO Now that Zvfh is a distinct config option, the |
Got it! I'll make changes for
to
Because it is instruction-level change, I'll put it in #1170. |
It would be good to put this nice diagram in |
It probably does not matter here, and as far as I can tell, this only applies to |
b18d574
to
296f8cd
Compare
Sorry, I closed the PR by accident, I'll try to reopen it. |
enum clause extension = Ext_Zvl32b | ||
mapping clause extensionName = Ext_Zvl32b <-> "zvl32b" | ||
function clause hartSupports(Ext_Zvl32b) = sizeof(vlen_exp) >= 5 | ||
enum clause extension = Ext_Zvl64b | ||
mapping clause extensionName = Ext_Zvl64b <-> "zvl64b" | ||
function clause hartSupports(Ext_Zvl64b) = sizeof(vlen_exp) >= 6 | ||
enum clause extension = Ext_Zvl128b | ||
mapping clause extensionName = Ext_Zvl128b <-> "zvl128b" | ||
function clause hartSupports(Ext_Zvl128b) = sizeof(vlen_exp) >= 7 | ||
enum clause extension = Ext_Zvl256b | ||
mapping clause extensionName = Ext_Zvl256b <-> "zvl256b" | ||
function clause hartSupports(Ext_Zvl256b) = sizeof(vlen_exp) >= 8 | ||
enum clause extension = Ext_Zvl512b | ||
mapping clause extensionName = Ext_Zvl512b <-> "zvl512b" | ||
function clause hartSupports(Ext_Zvl512b) = sizeof(vlen_exp) >= 9 | ||
enum clause extension = Ext_Zvl1024b | ||
mapping clause extensionName = Ext_Zvl1024b <-> "zvl1024b" | ||
function clause hartSupports(Ext_Zvl1024b) = sizeof(vlen_exp) >= 10 | ||
// Vector Extensions for Embedded Processors | ||
enum clause extension = Ext_Zve32f | ||
mapping clause extensionName = Ext_Zve32f <-> "zve32f" | ||
function clause hartSupports(Ext_Zve32f) = config extensions.Zve32f.supported | ||
enum clause extension = Ext_Zve32x | ||
mapping clause extensionName = Ext_Zve32x <-> "zve32x" | ||
function clause hartSupports(Ext_Zve32x) = config extensions.Zve32x.supported | ||
enum clause extension = Ext_Zve64d | ||
mapping clause extensionName = Ext_Zve64d <-> "zve64d" | ||
function clause hartSupports(Ext_Zve64d) = config extensions.Zve64d.supported | ||
enum clause extension = Ext_Zve64f | ||
mapping clause extensionName = Ext_Zve64f <-> "zve64f" | ||
function clause hartSupports(Ext_Zve64f) = config extensions.Zve64f.supported | ||
enum clause extension = Ext_Zve64x | ||
mapping clause extensionName = Ext_Zve64x <-> "zve64x" | ||
function clause hartSupports(Ext_Zve64x) = config extensions.Zve64x.supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it’s just me, but I think we should either let the user enable these extensions by explicitly setting vlen and (!) elen, or we implicitly set vlen and elen when using the Zve* and Zvl* extensions (like Spike does) and remove vlen and elen from the config file. Right now, this approach feels like a mix of both options, and elen doesn’t play a role at all anymore.
For example, if I set elen to 32 but enable Ext_Zve64x, what should elen actually be?
I also think the way currentlyEnabled(Ext_Zve*) is structured needs to change. For instance, enabling Zve64d should automatically enable all the Zve* subset extensions.
(Actually, we don’t need to enable all subset extensions, otherwise, the ISA string becomes unnecessarily long but at the very least, the user should be able to enable Zve64d without having to enable every subset extension manually)
Spike just updated its approach as well. Spike will set elen (and vlen if no Zvl* extension is provided) based on the Zve* extensions. So elen comes from Zve* and vlen from Zvl* or from Zve* (if no Zvl* is provided)
This is probably worth discussing in the Monday meeting @pmundkur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this is worth further discussion. I don't think we can remove VLEN as a config option and just use the Zvl* extensions though because that prevents anyone from using a VLEN greater than 1024. I know there are already implementations with VLEN greater than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, Spike supports Zvl2048b and Zvl4096b, even though they haven’t been ratified yet. We might want to do the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wwwwwwOwO We discussed this in the meeting today. The overall conclusion was to remove ELEN
from the config and instead set it to match whichever Zve
* extension is enabled, basically like Spike, taking the maximum possible ELEN. For example, if Zve64f
, Zve64d
,Zve64x
or V
are enabled, set ELEN=64
otherwise for the other extensions, set ELEN=32
.
But this will break the vector test case configurations in CI.
See
- https://github.com/riscv/sail-riscv/blob/master/config/CMakeLists.txt
(I just realized we currently generate config files where XLEN != ELEN, which is not supported.) - https://github.com/riscv/sail-riscv/blob/master/test/CMakeLists.txt
The good news is vector test cases are only supported when XLEN == ELEN. So, we could just replace ELEN with XLEN, and it should work.
If we decide to remove ELEN, I can patch these files myself, so you wont need to touch them.
This PR focuses on the configuration aspect extracted from #982, the features:
riscv_extensions.sail
, ordered by category and then alphabeticallyriscv_validate_config.sail
andriscv_vext_control.sail
Follow-up PRs will address instruction-level support logic.