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

make parameters for util modules public #1452

Merged

Conversation

albertchen-sifive
Copy link
Contributor

Related issue:
chipsalliance/rocket-chip#2452 (comment) The parameters of modules like Queue are useful for generating better names, but it is not possible to generate a fully unique name because parameters like pipe and flow are private.

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes

make parameters of modules in chisel3.util public

@albertchen-sifive albertchen-sifive requested a review from a team as a code owner May 29, 2020 18:44
@CLAassistant
Copy link

CLAassistant commented May 29, 2020

CLA assistant check
All committers have signed the CLA.

@seldridge
Copy link
Member

Would it make sense to override the desired name in these modules directly based on these parameters?

@jackkoenig
Copy link
Contributor

jackkoenig commented May 29, 2020

Would it make sense to override the desired name in these modules directly based on these parameters?

That's been my thinking. It also suggests we should provide some standard, structural serialization of arbitrary Data types (not hardware values). A "prettyPrinting" would be nice, and would work fine for small things, eg. UInt(8.W) -> U8, but for deep Bundles wouldn't be ideal (it could get very long, very fast and so eventually you'd need to hash it). You could allow Bundles to override it in a way similar to overriding desiredName in modules, but then there's risk users could cause collisions. Perhaps worth it to get a name like TLBundle_paramA_paramB_paramC instead of 7fc4e38

seldridge
seldridge previously approved these changes Jun 2, 2020
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed at the dev meeting. This looks good. Can you also make the gen parameter public? That may be useful in generating the name via serialization of the data type.

If you have good Chisel utility names, can you upstream those in a follow-up PR?

Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggesting changes for @seldridge 's reqest to make the gen also val

src/main/scala/chisel3/util/Arbiter.scala Outdated Show resolved Hide resolved
src/main/scala/chisel3/util/Arbiter.scala Outdated Show resolved Hide resolved
src/main/scala/chisel3/util/Decoupled.scala Outdated Show resolved Hide resolved
src/main/scala/chisel3/util/Valid.scala Outdated Show resolved Hide resolved
@seldridge seldridge dismissed their stale review June 7, 2020 01:19

Dismissing my review to wait on @mwachs5 changes.

albertchen-sifive and others added 2 commits July 28, 2020 10:39
make gen public

Co-authored-by: Megan Wachs <megan@sifive.com>
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@jackkoenig jackkoenig merged commit c613c28 into chipsalliance:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants