-
Notifications
You must be signed in to change notification settings - Fork 281
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
[FIRRTL] Lower non-trivial memory latencies to RTL #585
Conversation
4be8e6e
to
4d4d72a
Compare
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 have a broader comment that's outside the scope of this PR. This isn't the first time a CIRCT component has needed to add pipeline registers like this. Around memories, Handshake has to do exactly this, in the control network. Maybe there could be some shared utility if this is a common building block that CIRCT developers reach for.
4d4d72a
to
1c15cde
Compare
@mikeurbach wrote:
I agree. If the utility is purely for generating pipes in RTL (or comb + sequential) / SV, then I think that's pretty straightforward to generalize. If it's a more general utility that's abstract in the dialect that it targets, that sounds even more useful, but I'm not 100% sure how to go about it. Could this be implemented as a Dialect Interface? |
I was imagining something that just targets RTL/Comb/Seq/SV directly right now. I will open an issue about this. |
e092e1f
to
a22c777
Compare
Add lowering of memories with non-zero read latency and non-unary write latency using the strategy of the Scala FIRRTL Compiler (SFC). If memories have read or write ports with this property, then add delay pipes that delay the read or write for the expected number of cycles. This deviates slightly from the SFC behavior and creates aggregates for the pipes as opposed to one element for each stage of the pipe. Remove incomplete support for aggregate memory lowering. Before this commit, memories would be split, but there was no logic to actually handle these memories. Change this so that aggregate memories are an error in lowering and tell the user how to run FIRRTL type lowering. The existing register initialization logic is extended to support arrays (as this is required for the pipeline registers). Update tests and add a new test to check the pipeline register behavior. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
a22c777
to
7ee5aff
Compare
Fixes #477.
Add lowering of memories with non-zero read latency and non-unary
write latency using the strategy of the Scala FIRRTL
Compiler (SFC). If memories have read or write ports with this
property, then add delay pipes that delay the read or write for the
expected number of cycles. This deviates slightly from the SFC
behavior and creates aggregates for the pipes as opposed to one
element for each stage of the pipe.
Remove incomplete support for aggregate memory lowering. Before this
commit, memories would be split, but there was no logic to actually
handle these memories. Change this so that aggregate memories are an
error in lowering and tell the user how to run FIRRTL type lowering.
The existing register initialization logic is extended to support
arrays (as this is required for the pipeline registers).
Update tests and add a new test to check the pipeline register
behavior.
While large read and write latencies are arguably something that we could choose to not support, the standard sequential read memory (read latency 1, write latency 1) needs to have some lowering if #493 isn't run. This PR just solves the general case and aligns the behavior with SFC lowering.
I believe that removing the vestigial, incomplete memory lowering during FIRRTL to RTL conversion is justified because: (1) we should aim to do things correctly once and not have multiple different paths to produce the same effects and (2) FIRRTL to RTL conversion should eventually lower memories as aggregates anyway (structs/vectors should be preserved here). I don't think we're losing anything by ripping out the vestigial support and pushing people towards FIRRTL type lowering.
This PR synergizes with memory blackboxing #493 and addition of generator ops #547. This PR is the fallback path if memories aren't blackboxed or the logic can be reused for an eventual, default memory lowering/memory generator expansion pass.
Example
Consider the following FIRRTL IR (this is the example added in the tests). This has read and write latencies of 2:
If you compile this with the SFC, you get the read delayed by two cycles and the write delayed by one cycle:
With this PR, you get the following from CIRCT (
firtool -enable-lower-types -lower-to-rtl -verilog
):