-
Notifications
You must be signed in to change notification settings - Fork 597
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
Implement DecoupledIO.map utility #2646
Conversation
|
||
implicit class DecoupledExtensions[A <: Data](x: DecoupledIO[A]) { | ||
def map[B <: Data](f: A => B): DecoupledIO[B] = { | ||
val res = f(x.bits) |
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.
What if this were called "bits" and wire was called "res"? Just looking at the names of the resulting signals
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.
Or "res_bits" and "res"
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.
Alternatively, it could just be wire.bits := f(x.bits)
, or I think I like map
better as a name because it indicates that this came from calling .map
.
Also, this might be a good opportunity to utilize #2580 by using _
as the leading character for names in this block (wire
in particular isn't a super useful name in the Verilog, but map
could be...).
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.
Use of the leading underscore seems good to me. Maybe _map
or _mapped
. Alternatively, don't create a val res
here and just inline it. That'll create an unnamed node and that should get inlined? That would then motivate changing val wire
to val _map
.
Can you generate some examples of what the output looks like in Verilog for this?
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 forward to using this.
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.
Minor suggestions for improvements to the tests, but otherwise, this looks great!
Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>
(cherry picked from commit b20df1d)
Addresses #1072: Implements a new utility for
Decoupled
which applies a function to its data.Contributor Checklist
docs/src
?Type of Improvement
API Impact
Decoupled.map
to apply a function to aDecoupledIO
and return the new resultingDecoupledIO
Backend Code Generation Impact
Desired Merge Strategy
Release Notes
Implement
DecoupledIO.map
to apply a function toDecoupledIO.bits
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
.