chisel
chisel copied to clipboard
chipsalliance
We have no documentation/training on `Mem`
Type of issue: documentation
Impact: no functional change
Development Phase: request
Other information
If the current behavior is a bug, please provide the steps to reproduce the problem:
We don't have any documentation that I can find on Mem in the Chisel bootcamp, or the website explanations docs or cookbooks. We only cover SyncReadMem.
What is the current behavior?
What is the expected behavior?
We should probably at least mention this in the explanations section
Please tell us about your environment:
What is the use case for changing the behavior?
Not having undocumented features
@michael-etzkorn pointed out on gitter:
https://inst.eecs.berkeley.edu/~cs250/sp17/handouts/chisel-tutorial.pdf has info on Mem in 12.2 - - this could be added to chisel docs
I agree that what the tutorial has there is exactly what we should add to the Memories section of Chisel Docs. We should make it clear why people might want to use Mem vs SyncReadMem, and right now we only explain the latter really in the docs
Hm, actually we do mention this very briefly here, but with no examples: https://github.com/chipsalliance/chisel3/blob/master/docs/src/explanations/memories.md#mem-combinationalasynchronous-read-sequentialsynchronous-write
It seems like the documentation was adapted from the tutorial paper, but replaced Mem with SyncReadMem and expanded the examples to include module context. The Queue util has a decent example of instantiating SyncReadMem vs. Mem. Timely enough, today I first-hand experienced combinational path issues with register banks to output buffers due to Mem vs. SyncReadMem in the Queue util. While using the default useSyncReadMem = false for queues, the combinational asynchronous read in the queue caused some timing error not caught by DRC. I could work around it with a slower clock or with an ILA probe on the register that drove dequeue ready, but switching to SyncReadMem fixed the problem completely. I can only imagine queue is defaulted to useSyncReadMem= false for compatibility purposes.
After experiencing that issue with Mem, I think the part about FPGA memories inferred as register banks vs. SRAMs could be emphasized. After I poke around in the gate-level netlists and Vivado logs in my Mem vs. SyncReadMem use case, I'll hopefully be able to illustrate how asynchronous reads are potentially problematic with synthesis tools (or at the very least Vivado) and add some clarity there in the documentation.
i would love to make the documentation on the repo in md format if you guys approve and help a little
Hi @17Moons, thanks for your interest in contributing!
You should first take a look at our Contributing guide: https://github.com/chipsalliance/chisel/blob/main/CONTRIBUTING.md
Next, take a look at the README for how documentation is written here: https://github.com/chipsalliance/chisel/tree/main/docs.
This particular issue is a little out of date in that there is now some documentation for Mem, you can see it on the website: https://www.chisel-lang.org/docs/explanations/memories#mem-combinationalasynchronous-read-sequentialsynchronous-write.
That being said, that entire page could use some work--the sections are in a weird order, masks and memory initialization should go under SyncReadMem, rather than Mem sort of sitting between them. Mem could also use an example and some discussion similar to what Michael mentioned above.
You can edit the page by hitting the Edit this page button at the bottom, or by forking this repository and editing the corresponding markdown file https://github.com/chipsalliance/chisel/blob/main/docs/src/explanations/memories.md (note that Edit this page is just a link to that file).
(Repost because Github is not showing my edits to the original comment for some reason).
