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

Add Module.currentModule for getting a reference to the current Module #810

Merged
merged 1 commit into from
Apr 22, 2018

Conversation

jackkoenig
Copy link
Contributor

Resolves #809 and chipsalliance/rocket-chip#1363

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes

Add ability to get a reference to the current Module

Copy link
Contributor

@derekpappas derekpappas left a comment

Choose a reason for hiding this comment

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

currentModule is what we need.

@ducky64
Copy link
Contributor

ducky64 commented Apr 20, 2018

So the related issue (chipsalliance/rocket-chip#1363) says that there is a need to access the current module from within nested helpers, but doesn't give details. I'm curious what those are.

However, if we really do want the current module (rather than needing it as a proxy for something else), I think this API looks sane.

Copy link
Contributor

@ucbjrl ucbjrl left a comment

Choose a reason for hiding this comment

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

lgtm (and Jenkins)

Copy link
Contributor

@terpstra terpstra left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor

@terpstra terpstra left a comment

Choose a reason for hiding this comment

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

Great!

@jackkoenig
Copy link
Contributor Author

@ducky64 An example use case is the rocket-chip Reg(Field|Mapper) which constructs a TileLink memory map for memory-mapped registers in the design. Passing around the current module throughout some fairly sophisticated logic would be onerous, but it is useful to be able to attach information (ie. annotate) the current Module with this information.

@ducky64
Copy link
Contributor

ducky64 commented Apr 22, 2018

@jackkoenig I don't see a reference to Module in the linked file (and I don't quite understand the example), but mostly what I want to make sure in this PR is that we're not using this as a less-perfect proxy for some other more narrow but more relevant feature, like associating annotations with wires.

As your understanding of the use case is better than mine, if this is actually the feature you want, I have no objections.

@jackkoenig
Copy link
Contributor Author

@ducky64 Apologies for not being more clear. The linked code wants to use this API, it is in the process of being converted to use annotations in this PR chipsalliance/rocket-chip#1369

I agree with your concern in general. In this case, I think this API is really helpful.

@jackkoenig jackkoenig merged commit 297e9fa into master Apr 22, 2018
@jackkoenig jackkoenig deleted the current-module branch May 15, 2018 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants