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

Deprecate std_mem #1261

Closed
1 task
rachitnigam opened this issue Nov 13, 2022 · 7 comments
Closed
1 task

Deprecate std_mem #1261

rachitnigam opened this issue Nov 13, 2022 · 7 comments
Labels
C: Library Calyx's standard library Status: Available Can be worked upon

Comments

@rachitnigam
Copy link
Contributor

rachitnigam commented Nov 13, 2022

Standard memories are, in general, not synthesizable to BRAMs or URAMs on FPGAs because of combinational reads. The new sequential memories provide sequential, latency-insensitive interfaces to memories and are amenable to mapping optimizations like #1151.

In addition to #907, we should move the std_mem primitives into the unsynthesizable.futil file to signal that they cannot be easily synthesized and used

TODO

  • Change CIRCT to use seq memories
@rachitnigam rachitnigam added Status: Discussion needed Issues blocked on discussion C: Library Calyx's standard library labels Nov 13, 2022
@andrewb1999
Copy link
Collaborator

Before we do this we should add seq_mems to CIRCT and move passes over to use them instead.

@rachitnigam
Copy link
Contributor Author

@cgyurgyik thoughts on how hard this would be? The difference is that reads from memories require setting read_en signal and waiting till read_done is high before reading the value

@cgyurgyik
Copy link
Collaborator

I imagine the brunt of it will come from refactoring. The PipelineToCalyx pass may be trickier, depending on how specialized memory accesses are. I don't recall exactly.

@rachitnigam
Copy link
Contributor Author

We can, for now, leave the std_mem in the repo when we do this and write a short pass to emit a warning whenever they are used by a frontend. We can eventually remove them completely once CIRCT stops using them

@rachitnigam rachitnigam added Status: Available Can be worked upon and removed Status: Discussion needed Issues blocked on discussion labels Jan 1, 2023
@rachitnigam
Copy link
Contributor Author

Linking a conversation that motivated this: #1221

@cgyurgyik
Copy link
Collaborator

I think effort towards this was made on the CIRCT side in llvm/circt#5471.

@rachitnigam
Copy link
Contributor Author

#1901 renamed std_mem_* memories to comb_mem_* to indicate that they have combinational reads. I'm going to consider this sufficient to close this PR. In the future, we can consider removing these memories but for now, we should systematically eliminate their use from the code base.

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 Status: Available Can be worked upon
Projects
None yet
Development

No branches or pull requests

3 participants