-
Notifications
You must be signed in to change notification settings - Fork 676
Add Zcb and Zcmp extension RTL #2347
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
Conversation
ef2ce5f to
11d7d7a
Compare
andreaskurth
left a comment
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.
LGTM, thanks @SamuelRiedel!
(For reference, see #2324 for DV and more detailed review comments.)
11d7d7a to
9e49508
Compare
marnovandermaas
left a comment
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.
Very cool pull request this. Thanks for all your hard work. I just had a few small comments based on a code review.
This is required to ensure we can trace all expanded instructions but also already advance the PC on the last expanded instruction
Tracking also the original instruction, not only the micro-op, allows the DV to track whether and which instruction we are expanding
The tracer usually only sees the instructions that reach the ID stage. Since the Zcmp instructions are expanded in the IF stage, they will be traced as their micro-ops. This adds information in the trace from which expanded instruction those micro-ops come from.
The handshake only considered whether the ID stage would be ready. But the actual pipeline register will also take the `pc_set_i` signal into account, which signals a jump. Since the compressed decoder has state now (through the Zcmp extension), this improper handshake led to some of the expanded instructions to get lost. At the same time, we also take this signal into account for the enable signal of the pipeline stage to avoid unnecessary switching.
Merge the two cm.mv* states into a single one. This should still be easy to understand and saves us an extra bit in the encoding.
9e49508 to
c9081f8
Compare
|
Thank you, @marnovandermaas. I implemented your feedback and ran the tests again. I will wait with the squashing until the end to make reviewing easier, since many of those commits are small bug fixes on top of Andreas's commit. |
|
The full test suite results with the new DV for this RTL can be found here: #2324 (comment) The full regression results of this PR with the current DV is the following. Note that the Ibex Regression Results
Coverage
|
This PR adds only the RTL part of #2324. We will merge the DV alongside the compiler update once we also upstreamed the riscv-dv changes.
This PR adds support for the Zcb and Zcmp code-saving extensions. This support is implemented in a parameterizable way via the RV32ZC parameter, which allows choosing none, either, or both extensions. By default, both extensions are enabled because their combined hardware overhead is small, approximately 800 gate equivalents.
The Zcb extension introduces new compressed encodings for common instructions already supported in Ibex. The Zcmp extension introduces single compressed instructions that expand into multiple existing instructions within Ibex. For example, a stack push expands into multiple store instructions and a stack pointer update. Therefore, both extensions are implemented in the IF stage.