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

Pass optional name in ImportDefinitionAnno used during Separate elaboration of definition and instance #2592

Merged

Conversation

girishpai
Copy link
Contributor

@girishpai girishpai commented Jun 21, 2022

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • code refactoring
  • new API

API Impact

  • No impact - since the default value of this option is None - and the previous behavior should hold

Backend Code Generation Impact

  • Change is relative to the override name given for the external definition. Previously it used to pick only definition.proto.name - now it can hold a different value provided by the user.

Technical Debt

  • Ideally the original issue of getting actual names of the external components in the final circuit can / should be solved more cleanly by making Prefixing a first class support within Chisel and/or having the concept of Namespaces within Firrtl.

Desired Merge Strategy

Squash

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@girishpai
Copy link
Contributor Author

@mwachs5 @azidar @jackkoenig - this PR addresses part of the problem when we have prefixing / name changes involved between different elaborations - i.e. the subsequent elaborations need to be passed "the final name in the circuit" of the definition. What this final name will be is a separate problem to solve (for example when there is a NestedModulePrefixing being done - then we need to figure out if this definition gets the prefix or not and pass the name accordingly in the definition. Hope this made sense.

High level this is what was done

  • Add an optional name parameter for ImportDefinitionAnnotation
  • Within the DynamicContext class populate a Map from definition.proto.name to this optional name. If no Name passed (aka it is None - then use the definition.proto.name` as before)
  • Query this Map to get the desiredName for the EmptyExtModule

Still working on adding more tests - feel free to comment / suggest changes or request for more clarifications.

@mwachs5
Copy link
Contributor

mwachs5 commented Jun 21, 2022

In general, I think we'd be better served to re-visit #2155 and fix its limitations. What do you think? I'll comment on that PR

@girishpai
Copy link
Contributor Author

@mwachs5 - agreed. However my main concern is that path might take longer depending on the challenges we face. Adding support to pass extModName for the external definition - might be useful to come up with adhoc solutions tackle with prefixing + multiple elaborations.

@girishpai girishpai marked this pull request as ready for review June 21, 2022 20:48
@girishpai girishpai requested review from mwachs5 and azidar June 21, 2022 20:48
@girishpai
Copy link
Contributor Author

@mwachs5 @jackkoenig @azidar - please re-review when you all can!

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

One nit but overall LGTM

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
@girishpai
Copy link
Contributor Author

@jackkoenig FYI we would need this backported to 3.5 (assuming that's what we use internally). Not sure who is going to take care of that.

@jackkoenig jackkoenig added this to the 3.5.x milestone Jun 22, 2022
@jackkoenig jackkoenig enabled auto-merge (squash) June 22, 2022 00:52
@jackkoenig jackkoenig merged commit 48d57cc into chipsalliance:master Jun 22, 2022
mergify bot pushed a commit that referenced this pull request Jun 22, 2022
Used for separate elaboration of Definition and Instance

(cherry picked from commit 48d57cc)
@mergify mergify bot added the Backported This PR has been backported label Jun 22, 2022
mergify bot added a commit that referenced this pull request Jun 22, 2022
Used for separate elaboration of Definition and Instance

(cherry picked from commit 48d57cc)

Co-authored-by: Girish Pai <girish.pai@sifive.com>
jackkoenig added a commit that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants