-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use a custom type to represent clusters instead of an array #48
Conversation
Introduce a `Cluster` type that represents a cluster of register blocks, with an `Index` impl that computes the address of the register we're interested in, instead of computing all the register addresses up front to store them in an array. This provides a significant improvement in code size (see Infineon#46). We have to do some trickery in order to support subscripting, because the `Index` trait only supports indexes returing references. This commit implements the solution proposed by @pellico: represent a register block within a cluster as a reference to a zero-sized type. Any non-null, sufficiently aligned reference to a ZST is valid, so we can forge references to ZSTs and use the address of the reference to represent the address of the register block. This is sound as long as we never expose an *owned* ZST to safe code, and we mark the cluster ZST types as `#[non_exhaustive]` so users can't construct their own.
I don't have any additional comments - my only thought was that as-is we don't have a direct way to get the address of a cluster (e.g. to pass to C). But that is easily worked around by getting the address from the first register. |
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.
Looks great, thanks!
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.
damn, wrong user before...
Description
Introduce a
Cluster
type that represents a cluster of register blocks, with anIndex
impl that computes the address of the register we're interested in, instead of computing all the register addresses up front to store them in an array. This provides a significant improvement in code size.We have to do some trickery in order to support subscripting, because the
Index
trait only supports indexes returning references. This commit implements the solution proposed by @pellico: represent a register block within a cluster as a reference to a zero-sized type. Any non-null, sufficiently aligned reference to a ZST is valid, so we can forge references to ZSTs and use the address of the reference to represent the address of the register block. This is sound as long as we never expose an owned ZST to safe code, and we mark the cluster ZST types as#[non_exhaustive]
so users can't construct their own.Code size impacts
Recompiling a small test application with this change reduced code size from 13388 bytes to 12104 bytes. Adding a couple of strategic
get_unchecked
calls within the GPIO driver (where it is safe to do so because the port number is checked at compile time) reduced code size further to 11700 bytes. That's a 13% reduction in code size!The following is a screenshot of code generated by the compiler before this change, configured to optimize for size:
This function sets a single field in a GPIO configuration register. Just reading the register's previous value took 106 bytes of machine code, and then the compiler emitted a separate 134-byte function to write the new value back to the register. That's 240 bytes, just to change 2 bits of a register.
Here's the same function after this change:
Down to 40 bytes! And it can get even smaller: if the port and pin number are statically known, the compiler can constant-fold and issue instructions to directly access the register's memory address instead of having to compute it.
Documentation impacts
Not nearly as I feared, since the ZST reference trick is only needed for clusters structs, not every single peripheral struct.
Here's what rustdoc looked like before:
And here's after:
Compatibility impacts
This change is almost entirely source-compatible. However, some minor source changes may be required because Rust tries to automatically dereference the result of an indexing operation. If a user accesses
cluster()[i]
and stores the result in a variable or returns it from a function, they may need to add an&
or callcluster().get(i)
in order to reference the register instead of trying to move it. The error message is clear and the compiler suggests a fix:Related Issue
Fixes #46
--- please keep the agreement as part of the PR text ---
By creating this pull request you agree to the terms in CONTRIBUTING.md.
https://github.com/Infineon/svd2pac/blob/main/CONTRIBUTING.md
CONTRIBUTING.md also tells you what to expect in the PR process.