-
Notifications
You must be signed in to change notification settings - Fork 160
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
riscv: add CSR-defining macros #219
base: master
Are you sure you want to change the base?
Conversation
riscv/src/register/tests.rs
Outdated
read_write_csr! { | ||
"test CSR register type", | ||
Mtest: 0x000, | ||
0b1111_1111, | ||
"test single-bit field", | ||
single: 0, | ||
"setter test single-bit field", | ||
set_single: 0, | ||
} | ||
|
||
read_write_csr_field! { | ||
Mtest, | ||
"multiple single-bit field range", | ||
multi_range: 1..=3, | ||
"setter multiple single-bit field range", | ||
set_multi_range: 1..=3, | ||
} | ||
|
||
read_write_csr_field!( | ||
Mtest, | ||
"multi-bit field", | ||
multi_field: [4:7], | ||
"setter multi-bit field", | ||
set_multi_field: [4:7], | ||
); |
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.
Test calls to the new macros. The above is just a basic compile test.
Still need to add unit tests, and macro calls for the read-only
and write-only
variants.
Thanks! I'll review it ASAP |
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.
Kind of like it! the mask idea is very nice. I miss two main things:
- Less repetition in macros. Maybe we could atomize them and leave one "core" macro that creates the struct with bitmask and from_bits/bits, another for the get/set...
- Using custom types (mainly enums) for setters and getters. That would be awesome (for example XLEN
riscv/src/register/macros.rs
Outdated
macro_rules! read_write_csr { | ||
($doc:expr, | ||
$ty:ident: $csr:tt, | ||
$mask:tt, | ||
$( | ||
$field_doc:expr, | ||
$field:ident: $get_bit:literal, | ||
$set_field_doc:expr, | ||
$set_field:ident: $set_bit:literal$(,)? | ||
)+) => { | ||
$crate::read_write_csr!($doc, $ty: $csr, $mask); | ||
|
||
$( | ||
$crate::read_write_csr_field!( | ||
$ty, | ||
$field_doc, | ||
$field: $get_bit, | ||
$set_field_doc, | ||
$set_field: $set_bit, | ||
); | ||
)+ | ||
}; |
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.
Less repetition in macros. Maybe we could atomize them and leave one "core" macro that creates the struct with bitmask and from_bits/bits, another for the get/set...
That's essentially what is there, each of the branches for the various ways to call [read,write]_[only,write]_csr
call the internal branch for creating the struct.
Then, the repeated part (in the $(...)+
) creates the getters/setters. I guess we could also make read_write_csr_field
into another branch of the macro...
Other than that, I'm not sure exactly what you're asking for. What changes would you like to see?
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.
What I mean is that, for example, this part of the macro where the register struct is defined appears three times. It would be nice to have it somehow just once in a separate, basic macro and, depending on the macro branches, call this macro
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.
Ah, I see what you mean. Because each of the macros has this helper branch, make it into its own macro that each one could call.
Alright, I'll work on separating it out. Maybe with a note that you can, but probably don't want to, call this helper macro on its own.
Edit: I added the changes as a fixup
commit for easier review. If you like the approach, I'll squash the commit into the previous one.
We could add another branch that takes an I can try to work something out. |
I added the Let me know what you think. |
@rmsyn I'm on it, but macros are difficult to review... 😥 Looks great. The only thing now is that I prefer to leave some macros out of the equation, and then let us call the proper sequence of macros according to the CSR. I think this will help us in the future to deal with macros (modular vs monolithic). Regarding Great job!! I'll keep studying all your code 😁 |
riscv/src/register/macros.rs
Outdated
$field:ident: $get_bit:literal, | ||
$set_field_doc:expr, | ||
$set_field:ident: $set_bit:literal$(,)? |
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.
Another thing I would like your opinion on @romancardenas, do you think there are use-cases where users would want to define different bits for the get
and set
functions?
I couldn't really think of a place where this would be a good idea, so maybe the bit
argument could be a single parameter?
Same idea for the range and multi-bit variants.
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.
I think these macros should work in the general use cases (i.e., the same bits for getters and setters). For particular cases, we can always just implement them by hand.
Adds helper macros for defining CSR types.
Adds the ability to define CSR types using an enum field type.
Adds corrections to CSR creation macros for rebase on commit `a4d69614`. Includes: - additions of `try_*` function variants for fallible functions. - changing doc string macro arguments to idiomatic `///` comments
31959be
to
e4b5b12
Compare
Updated the changes onto the latest HEAD commit. Squashed the previous fixup commit to simplify review. Added the necessary changes to incorporate the addition of fallible functions, and changed the doc-string arguments. Let me know if you like the changes. I still think the bit-field and bit-range macro arguments could be simpler, especially now with four versions of essentially the same macro argument (e.g. Currently, it feels like having the user supply the same argument past the colon all four times is a potential footgun. Instead, we could change the |
Replaces repeated field bit and range arguments with a single `bit` or `range` argument. Intended to reduce copy-paste, prevent typo errors, and clarify intent.
Adds `try_*` variants for fallible getter functions on read-only CSR macros.
Adds `try_*` variants for fallible getter functions on write-only CSR macros.
Modularizes the `read_only_csr_field` and `write_only_csr_field` macro helpers for re-use in `read_write_only_csr_field`.
Adds the explicit `mask:` field name to CSR helper macros to clarify intent.
Adds the `field` macro argument label to CSR helper macros to clarify intent.
@romancardenas I added some more fixes + improvements to the macro helpers. Reduced some redundancy, and hopefully made the macro invocations more readable. Please let me know what you think. More or less, this PR is probably pretty close to "finished"(tm). Let me know if there are any more changes you would like to see. |
I'll try to review this later today. Thanks! |
Looks good, but it is true that having to do something like: field: {
/// Gets the `cycle[h]` inhibit field value.
cy,
/// Attempts to get the `cycle[h]` inhibit field value.
try_cy,
/// Sets the `cycle[h]` inhibit field value.
///
/// **NOTE**: only updates the in-memory value without touching the CSR.
set_cy,
/// Attempts to set the `cycle[h]` inhibit field value.
///
/// **NOTE**: only updates the in-memory value without touching the CSR.
try_set_cy,
bit: 0,
}, is perhaps too verbose. Can't we do something like: field: {
/// `cycle[h]` inhibit field value
cy,
bit: 0,
}, And then generate the same code? Just following the Also, I feel like there are a lot of branches with similar code. Can't we simplify the macros using recursive calls that match patterns, consuming part of the tokens? I think in this way we would have simpler pattern matching and less duplicates. |
Yes, this is what introducing the Personally, using the crate dependency is the better option, as it is actively maintained + high code quality. I really do think the use-case justifies the additional dependency. I'll add a commit using the
Could you make comments on the specific places where you think they could be refactored into recursive branches? I have a couple of those branches in the |
riscv/src/register/macros.rs
Outdated
|
||
($ty:ident, | ||
$(#[$field_doc:meta])+ | ||
$field:ident: [$bit_start:literal : $bit_end:literal]$(,)?) => { | ||
impl $ty { | ||
paste::paste! { | ||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn $field(&self) -> usize { | ||
self.[<try_ $field>]().unwrap() | ||
} | ||
|
||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn [<try_ $field>](&self) -> $crate::result::Result<usize> { | ||
let max_width = core::mem::size_of_val(&self.bits) * 8; | ||
|
||
if $bit_end < max_width && $bit_start <= $bit_end { | ||
Ok($crate::bits::bf_extract(self.bits, $bit_start, $bit_end - $bit_start + 1)) | ||
} else { | ||
Err($crate::result::Error::IndexOutOfBounds { | ||
index: $bit_start, | ||
min: $bit_start, | ||
max: $bit_end, | ||
}) | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
|
||
($ty:ident, | ||
$(#[$field_doc:meta])+ | ||
$field:ident: { | ||
$(#[$field_ty_doc:meta])+ | ||
$field_ty:ident { | ||
range: [$field_start:literal : $field_end:literal], | ||
default: $default_variant:ident, | ||
$($variant:ident = $value:expr$(,)?)+ | ||
}$(,)? | ||
}$(,)? | ||
) => { | ||
$crate::csr_field_enum!( | ||
$(#[$field_ty_doc])+ | ||
$field_ty { | ||
range: [$field_start : $field_end], | ||
default: $default_variant, | ||
$($variant = $value,)+ | ||
}, | ||
); | ||
|
||
$crate::read_only_csr_field!( | ||
$ty, | ||
$(#[$field_doc])* | ||
$field: [$field_start : $field_end], | ||
$field_ty, | ||
); | ||
}; | ||
|
||
($ty:ident, | ||
$(#[$field_doc:meta])+ | ||
$field:ident: [$field_start:literal : $field_end:literal], | ||
$field_ty:ident$(,)? | ||
) => { | ||
impl $ty { | ||
paste::paste! { | ||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn $field(&self) -> $field_ty { | ||
self.[<try_ $field>]().unwrap() | ||
} | ||
|
||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn [<try_ $field>](&self) -> $crate::result::Result<$field_ty> { | ||
let max_width = core::mem::size_of_val(&self.bits) * 8; | ||
|
||
if $field_end < max_width && $field_start < $field_end { | ||
let value = $crate::bits::bf_extract( | ||
self.bits, | ||
$field_start, | ||
$field_end - $field_start + 1, | ||
); | ||
|
||
$field_ty::from_usize(value).ok_or($crate::result::Error::InvalidFieldVariant { | ||
field: stringify!($field), | ||
value, | ||
}) | ||
} else { | ||
Err($crate::result::Error::IndexOutOfBounds { | ||
index: $field_start, | ||
min: $field_start, | ||
max: $field_end, | ||
}) | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
} |
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.
I rewrote the read_only_csr_field
using the paste
macro.
Please let me know if you are alright adding the dependency, and I'll rewrite the rest of the macros.
Please be sure about this decision, as it is a lot of work rewriting these macros.
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.
Then let's cool this down until we resolve other design decisions. I can say that, as of now, I like how it looks
read_only_csr! { | ||
/// test CSR register type | ||
Mtest: 0x000, | ||
mask: 0b1111_1111_1111, | ||
/// test single-bit field | ||
single: 0, | ||
} | ||
|
||
read_only_csr_field! { | ||
Mtest, | ||
/// multiple single-bit field range | ||
multi_range: 1..=3, | ||
} | ||
|
||
read_only_csr_field!( | ||
Mtest, | ||
/// multi-bit field | ||
multi_field: [4:7], | ||
); | ||
|
||
read_only_csr_field!( | ||
Mtest, | ||
/// multi-bit field | ||
field_enum: { | ||
/// field enum type with valid field variants | ||
MtestFieldEnum { | ||
range: [7:11], | ||
default: Field1, | ||
Field1 = 1, | ||
Field2 = 2, | ||
Field3 = 3, | ||
Field4 = 15, | ||
} | ||
} | ||
); |
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.
You can see how much the paste
crate simplifies macro invocation, it is substantial.
The savings are even greater for the read_write_csr*
variants.
About reducing code size. Let's take only two pattern branches in #[macro_export]
macro_rules! read_only_csr {
($(#[$doc:meta])+
$ty:ident: $csr:tt,
mask: $mask:tt,
$(
$(#[$field_doc:meta])+
$field:ident: $bit:literal$(,)?
)+
) => {
$crate::csr! { $(#[$doc])+ $ty: $csr, $mask }
$(
$crate::read_only_csr_field! {
$ty,
$(#[$field_doc])+
$field: $bit,
}
)+
$crate::read_csr_as!($ty, $csr);
};
($(#[doc:meta])+
$ty:ident: $csr:tt,
mask: $mask:tt,
$(
$(#[$field_doc:meta])+
$field:ident: $bit:literal ..= $bit_end:literal$(,)?
)+) => {
$crate::csr! { $(#[$doc])+ $ty: $csr, $mask }
$(
$crate::read_only_csr_field! {
$ty,
$(#[$field_doc])+
$field: $bit ..= $bit_end,
}
)+
$crate::read_csr_as!($ty, $csr);
};
...
} Both branches do something like: $crate::csr! { $(#[$doc])+ $ty: $csr, $mask }
$(
$crate::read_only_csr_field! {
$ty,
$(#[$field_doc])+
<THIS LINE IS THE ONLY THING THAT CHANGES>
}
)+
$crate::read_csr_as!($ty, $csr); Looking at this specific example, the macro calls other macros ( |
riscv/src/register/macros.rs
Outdated
/// [read_only_csr](crate::read_only_csr), and [write_only_csr](crate::write_only_csr) macros. | ||
#[macro_export] | ||
macro_rules! csr { | ||
($doc:expr, $ty:ident: $csr:literal, $mask:literal) => { |
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.
We don't need to select the CSR number at this point
($doc:expr, $ty:ident: $csr:literal, $mask:literal) => { | |
($doc:expr, $ty:ident, $mask:literal) => { |
riscv/src/register/macros.rs
Outdated
$crate::read_csr_as!($ty, $csr); | ||
$crate::write_csr_as!($ty, $csr); | ||
$crate::set!($csr); | ||
$crate::clear!($csr); |
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.
I think I'd leave these macros out and call them as we need them
$crate::read_csr_as!($ty, $csr); | |
$crate::write_csr_as!($ty, $csr); | |
$crate::set!($csr); | |
$crate::clear!($csr); |
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.
I agree about leaving out the set
and clear
macros, but think the read_csr_as
and write_csr_as
should still be included.
Would there ever be a case where one would call:
read_write_csr
, but notread_csr_as
andwrite_csr_as
read_only_csr
, but notread_csr_as
write_only_csr
, but notwrite_csr_as
In my opinion, including these macro calls avoids an author forgetting to include them. If you really think they should be removed, I'll take them out.
riscv/src/register/macros.rs
Outdated
$field:ident: $get_bit:literal, | ||
$set_field_doc:expr, | ||
$set_field:ident: $set_bit:literal$(,)? |
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.
I think these macros should work in the general use cases (i.e., the same bits for getters and setters). For particular cases, we can always just implement them by hand.
riscv/src/register/macros.rs
Outdated
|
||
($ty:ident, | ||
$(#[$field_doc:meta])+ | ||
$field:ident: [$bit_start:literal : $bit_end:literal]$(,)?) => { | ||
impl $ty { | ||
paste::paste! { | ||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn $field(&self) -> usize { | ||
self.[<try_ $field>]().unwrap() | ||
} | ||
|
||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn [<try_ $field>](&self) -> $crate::result::Result<usize> { | ||
let max_width = core::mem::size_of_val(&self.bits) * 8; | ||
|
||
if $bit_end < max_width && $bit_start <= $bit_end { | ||
Ok($crate::bits::bf_extract(self.bits, $bit_start, $bit_end - $bit_start + 1)) | ||
} else { | ||
Err($crate::result::Error::IndexOutOfBounds { | ||
index: $bit_start, | ||
min: $bit_start, | ||
max: $bit_end, | ||
}) | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
|
||
($ty:ident, | ||
$(#[$field_doc:meta])+ | ||
$field:ident: { | ||
$(#[$field_ty_doc:meta])+ | ||
$field_ty:ident { | ||
range: [$field_start:literal : $field_end:literal], | ||
default: $default_variant:ident, | ||
$($variant:ident = $value:expr$(,)?)+ | ||
}$(,)? | ||
}$(,)? | ||
) => { | ||
$crate::csr_field_enum!( | ||
$(#[$field_ty_doc])+ | ||
$field_ty { | ||
range: [$field_start : $field_end], | ||
default: $default_variant, | ||
$($variant = $value,)+ | ||
}, | ||
); | ||
|
||
$crate::read_only_csr_field!( | ||
$ty, | ||
$(#[$field_doc])* | ||
$field: [$field_start : $field_end], | ||
$field_ty, | ||
); | ||
}; | ||
|
||
($ty:ident, | ||
$(#[$field_doc:meta])+ | ||
$field:ident: [$field_start:literal : $field_end:literal], | ||
$field_ty:ident$(,)? | ||
) => { | ||
impl $ty { | ||
paste::paste! { | ||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn $field(&self) -> $field_ty { | ||
self.[<try_ $field>]().unwrap() | ||
} | ||
|
||
$(#[$field_doc])+ | ||
#[inline] | ||
pub fn [<try_ $field>](&self) -> $crate::result::Result<$field_ty> { | ||
let max_width = core::mem::size_of_val(&self.bits) * 8; | ||
|
||
if $field_end < max_width && $field_start < $field_end { | ||
let value = $crate::bits::bf_extract( | ||
self.bits, | ||
$field_start, | ||
$field_end - $field_start + 1, | ||
); | ||
|
||
$field_ty::from_usize(value).ok_or($crate::result::Error::InvalidFieldVariant { | ||
field: stringify!($field), | ||
value, | ||
}) | ||
} else { | ||
Err($crate::result::Error::IndexOutOfBounds { | ||
index: $field_start, | ||
min: $field_start, | ||
max: $field_end, | ||
}) | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
} |
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.
Then let's cool this down until we resolve other design decisions. I can say that, as of now, I like how it looks
riscv/src/register/macros.rs
Outdated
pub fn $try_field(&mut self, $field: bool) -> $crate::result::Result<()> { | ||
let max_width = core::mem::size_of_val(&self.bits) * 8; | ||
|
||
if $bit < max_width { | ||
self.bits = $crate::bits::bf_insert(self.bits, $bit, 1, $field as usize); | ||
Ok(()) | ||
} else { | ||
Err($crate::result::Error::IndexOutOfBounds { | ||
index: $bit, | ||
min: 0, | ||
max: max_width, | ||
}) | ||
} |
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.
We can do this as a compile-time check, right? we do know the value of max_width
and $bit
, so we can do a compile-time check to verify that everything is OK and just use the pub fn $field
function, as we are sure that it will not fail.
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.
When working on this, I was considering the safety of the bits
functions. There should be try_
variants of the bf_insert
and bf_extract
functions, because both of these functions can implicitly panic.
We would use the checked_shl
function instead of <<
operator.
What would the compile-time check look like here, as an example?
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 $bit
param is fixed, right? For example, the bit number 3 of a register. Thus, we can add something like this to check at compile time that $bit
is within the expected bounds. In this way, we don't need try_
methods
riscv/src/register/macros.rs
Outdated
let max_width = core::mem::size_of_val(&self.bits) * 8; | ||
|
||
if index < max_width && ($bit_start..=$bit_end).contains(&index) { | ||
self.bits = $crate::bits::bf_insert(self.bits, index, 1, $field as usize); | ||
Ok(()) | ||
} else { | ||
Err($crate::result::Error::IndexOutOfBounds { |
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.
Here more or less the same. We need this try_field
method, as we don't know if the index will be valid. But we can be sure at compile time that $bit_start
and $bit_end
are correct, so we just need to check that index
is inside this range.
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.
Similarly as above, can you provide an example of what the compile-time check would look like?
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.
Same as above. We add compile-time checks for start
and end
. Then, we only need to check that the parameter index is within the expected range.
Hi @rmsyn ! I left you a few comments. Let's first figure out how we want to use the macros and then we can explore using the |
Ah, I see what you mean now. Yes, the code could be simplified by not trying to include field definitions in the type-creation macro. The main instance where including field definitions in the type-creation macro is when all fields are the same type (bit, range, field-mask, etc.). Otherwise, users will need to invoke the field macros anyway. I agree simplifying is the way to go here. |
Removes field arguments from CSR definition macros.
Removes the unused `csr` number argument from the `csr` macro.
Uses the crate result type for converting a CSR enum type from a `usize`.
I've moved the When we're settled on the other design work, I can bring that work into this branch. |
629e6a9
to
7ad499a
Compare
Applies compile-time checks in read-only CSR macros. Authored-by: rmsyn <github@weathered-steel.dev> Co-authored-by: romancardenas <rcardenas.rod@gmail.com>
Applies compile-time checks in write-only CSR macros. Authored-by: rmsyn <github@weathered-steel.dev> Co-authored-by: romancardenas <rcardenas.rod@gmail.com>
Adds helper macros for defining CSR types:
read-write
:read-only
:write-only
:Related to: #218