-
Notifications
You must be signed in to change notification settings - Fork 278
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
[Calyx] Add sequential memories #5471
Conversation
5632148
to
2527c73
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.
Broken documentation link but otherwise looks good
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.
Thanks!
dimension in `$sizes`. The `$width` attribute dictates the width of a single | ||
element. | ||
|
||
See https://capra.cs.cornell.edu/docs/calyx/libraries/core.html#memories for |
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.
This needs to be fixed here as well
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 know this PR has already received approvals, and I'm not going to block it if you really want to merge. But i think it's somewhat smelly code that the return values of this op isn't implemented in what i'd consider the canonical way to do this.
); | ||
|
||
let results = (outs | ||
Variadic<AnySignlessInteger>:$results |
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.
Really not a big fan of the long results type list (: i1, i32, i1, i1, i32, i1
) when most (all?) of those are known based on the memory type (size attributes).
Could change the results to
let results = (outs Variadic<AnySignlessInteger>:$addrs, AnySignlessInteger:$writeData, I1:$wrEn, I1:$wrDone, I1:$clk, AnySignlessInteger:$rdData, I1:$rdEn, I1:$rdDone)>
and using the InferTypeOpInterface
. This would also be much more canonical, in that all of the custom getters that you've implemented will be autogenerated.
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, but I am just following the method used on existing Calyx ops for now. This op functions very similarly to the existing calyx.memory
op. Maybe I can merge this PR and then work on another PR at a later date to bring all of the Calyx ops inline with the suggestions you make?
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.
Works for me - a general cleanup in this aspect would be nice!
I'm going to go ahead and merge this and then work on a PR that infers the types for all Calyx primitives, because I agree that the long list of types is pretty annoying. |
Adds new
calyx.seq_mem
op which, in contrast tocalyx.memory
, has sequential reads. Sequential reads on memories is much more realistic than combinational reads for most use cases. Native Calyx is planning to deprecatestd_mem
soon as many users are accidentally using that instead ofseq_mem
, which can significantly hurt the QOR.We will still need to change the LoopScheduleToCalyx pass to use
calyx.seq_mem
instead ofcalyx.memory
before deprecatingstd_mem
, which is something I am working on while adding loop schedule support for sequential loops.