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.

Sign up for GitHub

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

Jump to bottom

Added Force Name API #1634

Merged
merged 7 commits into from
Oct 26, 2020
Merged

Added Force Name API #1634

merged 7 commits into from
Oct 26, 2020

Conversation

azidar
Copy link
Contributor

@azidar azidar commented Oct 23, 2020 •
edited by chick
Loading

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

  • new feature/API

API Impact

Adds a new API - forcename.

Backend Code Generation Impact

This only changes generated Verilog names if the API is used. Otherwise, no changes.

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

New 'forcename' API enables enforcing the naming of signals or instances, even if a FIRRTL transform renames them.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@azidar azidar requested a review from a team as a code owner October 23, 2020 16:54
@azidar azidar requested review from chick and removed request for a team October 23, 2020 16:54
@chick chick added the Feature New feature, will be included in release notes label Oct 23, 2020
@azidar azidar requested a review from hcook October 23, 2020 17:04
chick
chick reviewed Oct 23, 2020
View reviewed changes
hcook
hcook approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@hcook hcook left a comment

Choose a reason for hiding this comment

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

I approve of the API and believe this will address my use case.

chick
chick approved these changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

I think this looks fine (with one quibble about the mdocs)
Based on the commits in it perhaps it needs a rebase

@chick chick added this to the 3.4.x milestone Oct 23, 2020
jackkoenig
jackkoenig requested changes Oct 23, 2020
View reviewed changes
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.

Overall looks good. Lots of nits and please rebase.

@colinschmidt
Copy link
Contributor

Is there a reason to not expect a new firttl ForceRename in the future that overrides this?
We've already done suggestName->Rename->ForceName so it seems like there is an inherent conflict between the designer wanting a certain name and transform writers wanting a different name.
I'm not opposed to this feature and would probably use it but just interested if there is some deeper problem of "who owns naming" here.

@azidar
Copy link
Contributor Author

azidar commented Oct 23, 2020

@colinschmidt This is the first time we've exposed a user-facing API which uses transforms to rename names. It is has the strongest semantics, meaning basically error if I can't do it. I'd see this as a core utility, so we wouldn't introduce a second way for users to rename things via a FIRRTL transform.

In general, it's best not to use this utility (it should be a last resort). However, in some cases you need backwards compatibility, and this enables more control/enforcement of names that was not supported by the existing naming mechanisms.

jackkoenig
jackkoenig approved these changes Oct 26, 2020
View reviewed changes
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.

LGTM!

@azidar azidar added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Oct 26, 2020
@mergify mergify bot merged commit 2d98132 into master Oct 26, 2020
mergify bot pushed a commit that referenced this pull request Oct 26, 2020
@azidar
Added Force Name API (#1634)
159e98c 
* Added forcename transform and tests

* Added documentation and additional error checking

* Added mdoc. Added RunFirrtlTransform trait

* Removed TODO comment

* Addressed reviewer feedback

* Removed trailing comma

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 2d98132)
@mergify mergify bot mentioned this pull request Oct 26, 2020
Merged
@mergify mergify bot added the Backported This PR has been backported label Oct 26, 2020
mergify bot added a commit that referenced this pull request Oct 26, 2020
@mergify @azidar
Added Force Name API (#1634) (#1638)
22e12bc 
* Added forcename transform and tests

* Added documentation and additional error checking

* Added mdoc. Added RunFirrtlTransform trait

* Removed TODO comment

* Addressed reviewer feedback

* Removed trailing comma

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 2d98132)

Co-authored-by: Adam Izraelevitz <azidar@gmail.com>
@jackkoenig jackkoenig mentioned this pull request Oct 30, 2020
Merged
@jackkoenig jackkoenig deleted the forcename branch July 7, 2021 03:44
@MAFOR-TECH MAFOR-TECH mentioned this pull request Oct 24, 2021
Closed
@sequencer sequencer mentioned this pull request Nov 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hcook hcook hcook approved these changes

@chick chick chick approved these changes

@jackkoenig jackkoenig jackkoenig approved these changes

Assignees

@hcook hcook

Labels
Backported This PR has been backported Feature New feature, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
3.4.x
Development

Successfully merging this pull request may close these issues.
5 participants
@azidar @colinschmidt @hcook @chick @jackkoenig