Add RISC-V extension constraints #63
Conversation
| constraint_value( | ||
| name = "riscv64", | ||
| constraint_setting = ":cpu", | ||
| ) | ||
|
|
||
| # Base integer instruction set, 64-bit, 16 registers. |
There was a problem hiding this comment.
Is there any opinion from the riscv expert about what the (inevitable) 64-bit 32-register core will be called? Or, to put it backwards, is the 'e' an official name or is it just what we have been using internally?
| package( | ||
| default_visibility = ["//visibility:public"], | ||
| ) | ||
|
|
There was a problem hiding this comment.
It feels like this should have a description of the intended usage. That you create a platform from the riscv base and add in the settings that are relevant to the particular silicon you have.
That is, I want to capture the design choice somewhere to serve as documentation.
There was a problem hiding this comment.
Yes, this needs documentation and examples. Possibly a README.md as well.
| licenses(["notice"]) | ||
|
|
||
| package( | ||
| default_visibility = ["//visibility:public"], |
There was a problem hiding this comment.
This needs
default_applicable_licenses = ["//:license"],
| @@ -0,0 +1,972 @@ | |||
| # Constraint settings describing RISC-V extensions. | |||
| licenses(["notice"]) | |||
There was a problem hiding this comment.
I think you can leave this out. It is a no-op for Bazel and now internally with Blaze.
| package( | ||
| default_visibility = ["//visibility:public"], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Yes, this needs documentation and examples. Possibly a README.md as well.
| ) | ||
|
|
||
| # Does the CPU possess extension M (multiplication and division)? | ||
| constraint_setting( |
There was a problem hiding this comment.
Normally I think macros are more work than help, but these are so regular that I think a macro which creates the setting and both constraints would be helpful.
I'm also not wild about this style for (effectively) boolean constraints, but that's the system we have. Any suggestions for https://docs.google.com/document/d/1LD4uj1LJZa98C9ix3LhHntSENZe5KE854FNL5pxGNB4/edit?usp=sharing about a better way to model these?
| constraint_setting = "Zicsr_setting", | ||
| ) | ||
|
|
||
| # Does the CPU possess extension Zifencei (Control and Status Register)? |
There was a problem hiding this comment.
How is this "Control and Status Register" different from :Zicsr, above?
|
|
||
| # Does the CPU possess extension M (multiplication and division)? | ||
| constraint_setting( | ||
| name = "M_setting", |
There was a problem hiding this comment.
I assume RISC-V publishes a canonical list of these, and what they mean. Can we link it here?
Also, I worry slightly about upper case target names given the existence of case-insensitive file systems (like on macos). In this case it seems unlikely to conflict, but we should probably stick with all lower case as a general rule.
| default_constraint_value = ":no_Zbc", | ||
| ) | ||
|
|
||
| # CPU does possess the B extension |
There was a problem hiding this comment.
Definitely a macro would help here, the comments are out of sync with the target names.
| ) | ||
|
|
||
| # CPU does not possess the Zihintpause extension | ||
| constraint_value( |
There was a problem hiding this comment.
Having stopped being able to read this, it'd be nice if we had a process to generate this file from a canonical source and check that in, rather than have someone go though and update by hand. Possibly a macro would make things enough clearer, though, that that wouldn't be needed.
| constraint_value( | ||
| name = "riscv32", | ||
| constraint_setting = ":cpu", | ||
| ) | ||
|
|
||
| # Base integer instruction set, 32-bit, 16 registers. | ||
| constraint_value( | ||
| name = "riscv32e", |
There was a problem hiding this comment.
Is this (and the riscv64e, below) sufficiently different from riscv32 to need a new cpu constraint, or is it better modeled as :riscv32 plus a new extension?
Mini design document: https://docs.google.com/document/d/18pFUSWH0vN-R0AfIC5Wk5yWzE3Rhe0miDI5TgPRj2s4/edit#
This PR is intended for discussion, I don't think it's necessarily ready to be merged. (In particular, I'm not sure if privileged execution modes are correctly modeled; see the design document.)
@aiuto