Skip to content
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

Add dyn_mems as primitives #2111

Merged
merged 6 commits into from
Jun 7, 2024
Merged

Add dyn_mems as primitives #2111

merged 6 commits into from
Jun 7, 2024

Conversation

nathanielnrn
Copy link
Contributor

@nathanielnrn nathanielnrn commented Jun 6, 2024

This adds dyn_mems which are like seq_mems but do not guarantee one-cycle read/write latencies. See #2105 for motivation.

One thing that's occurred to me is dyn_mem seems mainly useful as a placeholder for various implementations/connections to external memories. As in, if we need the guaranteed 1-cycle latency given by seq_mem we can rely on the seq_mem implementation.

The SystemVerilog dyn_mem implementation is exactly the same as the seq_mem implementation. So it seems that as a standalone primitive dyn_mem lacks utility. It seems like we could only add a "wrapper" of dyn_mem as a primitive without any actual implementation behind it. Maybe this is a can of worms not worth opening right now, as I don't know how happy the compiler would be with the possibility of "empty primitives." I'd be happy to hear thoughts people have on this.

Closes #2105

@nathanielnrn nathanielnrn marked this pull request as draft June 6, 2024 16:08
@nathanielnrn nathanielnrn changed the title Add dyn_mems as primitives WIP: Add dyn_mems as primitives Jun 6, 2024
@nathanielnrn nathanielnrn changed the title WIP: Add dyn_mems as primitives Add dyn_mems as primitives Jun 6, 2024
@nathanielnrn nathanielnrn added the C: Library Calyx's standard library label Jun 6, 2024
@nathanielnrn nathanielnrn marked this pull request as ready for review June 6, 2024 16:12
@rachitnigam
Copy link
Contributor

#1151 is also related to this in case we want the compiler to decide the latency of the memories.

@sampsyo
Copy link
Contributor

sampsyo commented Jun 7, 2024

Thanks, @nathanielnrn, and thanks to @rachitnigam for calling up that extremely-relevant prior discussion. I am not exactly sure how to proceed on the larger question here—clearly, there is some real design work to be done for variable-latency memories (and other components). But for my money, I think we should probably just add this brute-force route as reflected in this PR now so we can make progress with the AXI business? Any objections to that as a short-term plan, @rachitnigam?

One reason to stick with this PR as it currently stands rather than making these dyn_mems "empty" as @nathanielnrn suggests:

It seems like we could only add a "wrapper" of dyn_mem as a primitive without any actual implementation behind it.

Is just so that we can actually execute programs as-is, without externalizing them. That is, even if it's impractical, it is nice to have a "shim" implementation of dyn_mem that has the same semantics as the more-realistic implementations.

@rachitnigam
Copy link
Contributor

Seems fine to me! And yeah, I think the design of virtual operators is going to be important in the future so let's not rush it here.

* add yxi support for dyn-mems in calyx-ir-utils

* make clippy happy

* add yxi test for dyn-mems

* formatting
@nathanielnrn nathanielnrn enabled auto-merge (squash) June 7, 2024 19:52
@nathanielnrn nathanielnrn merged commit bb73c16 into main Jun 7, 2024
18 checks passed
@nathanielnrn nathanielnrn deleted the dyn-mem branch June 7, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Library Calyx's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Introduce dyn_mem_d1 primitive?
3 participants