-
Notifications
You must be signed in to change notification settings - Fork 587
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
Enrich ShiftRegister with SyncReadMem-based implementation. #2891
Conversation
e1559d9
to
d363680
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.
After taking a look at the implementation of new ShiftRegister with use_sp_mem = true
, I think this is simply adding a new instance of mem
to increase ShiftRegister
to multiple bank?
Besides the nitpick on names and the usage of null
, I think this PR is actually "adding multiple banks support to ShiftRegister
."? if so, I think the new banks to add should be 2,4,8...
, rather than only being 2
.
macros/src/main/scala/chisel3/internal/sourceinfo/SourceInfoTransform.scala
Outdated
Show resolved
Hide resolved
* @param use_sp_mem single port or dual port SRAM based implementation | ||
* @param name name of SyncReadMem object | ||
*/ | ||
def apply[T <: Data](in: T, n: Int, en: Bool, use_sp_mem: Boolean, name: String): T = |
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.
How about using name: Option[String]
? null
in Scala is a bad design choice.
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.
Actually the whole idea is summarized in a recent issue. In other words, please have a look at slide 34: for larger (i.e., wider and deeper) shift registers it is much more power and area efficient to implement them using SRAM than with flip-flops. If dual-port SRAM is available (i.e., if such memory compiler exists) that is a straightforward path. If you want to be even more power and area efficient, or if dual-port memory compiler isn't available in the target technology, you can alternatively use two half-size single-port SRAMs in ping-pong configuration. This is well explained in, say, Chapter 9 (pp. 208-211) of the textbook OFDM Baseband Receiver Design for Wireless Communications by Tzi-Dar Chiueh and Pei-Yun Tsai. Even though it is certainly possible to have any number of banks, just as you've said, there is no real gain in power and area for anything larger than two due to (control) logic overhead.
By the way, thanks on the suggestion how to eliminate null
. You can review it again.
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 for the clear explanation! Would you mind renaming the arg to useDualSRAMPort
and default to false
or something similar and provide a documentation to which? That will be much clear to users.
23c192a
to
a4debfa
Compare
Co-authored-by: Jiuyang Liu <liu@jiuyang.me>
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.
Looking good! Please consider my suggestion on the parameter name
Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Jiuyang Liu <liu@jiuyang.me> Co-authored-by: Megan Wachs <megan@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 3382cab) # Conflicts: # src/test/scala/chiselTests/util/RegSpec.scala
Co-authored-by: Jiuyang Liu <liu@jiuyang.me> Co-authored-by: Megan Wachs <megan@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 3382cab)
Co-authored-by: Jiuyang Liu <liu@jiuyang.me> Co-authored-by: Megan Wachs <megan@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 3382cab)
Co-authored-by: Jiuyang Liu <liu@jiuyang.me> Co-authored-by: Megan Wachs <megan@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 3382cab)
Co-authored-by: Jiuyang Liu <liu@jiuyang.me> Co-authored-by: Megan Wachs <megan@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 3382cab)
Contributor Checklist
docs/src
?Type of Improvement
API Impact
This feature extends the current API preserving full backward compatibility.
Backend Code Generation Impact
If SyncReadMem-based Shift Register implementation is selected this will impact the generated Verilog code.
Desired Merge Strategy
No preference.
Release Notes
Supplement ShiftRegister with SyncReadMem-based implementation.
Reviewer Checklist (only modified by reviewer)
3.4.x
, [small] API extension:3.5.x
, API modification or big change:3.6.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.