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

Improve performance of LowerTypes renaming #2024

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Improve performance of LowerTypes renaming #2024

merged 1 commit into from
Dec 16, 2020

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Dec 15, 2020 •
edited
Loading

In addition to the added tests, I tested this with 3 different designs, diffing the Verilog and output annotation files and saw the results were identical. Also, pretty sure this is correct because I originally used the InstanceKeyGraph from after transformation in LowerTypes instead of before, and 2/3 of my designs failed.

Also, not very scientific, but a design that was running very slowly during renaming in LowerTypes had it's total FIRRTL runtime reduced from 36m45s to 20m45s, so a massive win.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • [NA] Did you update the FIRRTL spec to include every new feature/behavior?
  • 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 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

  • performance improvement

API Impact

This adds some new public utilities

Backend Code Generation Impact

No impact

Desired Merge Strategy

  • Squash

Release Notes

Improve performance and memory use of LowerTypes

Reviewer Checklist (only modified by reviewer)

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

@jackkoenig jackkoenig added this to the 1.4.x milestone Dec 15, 2020
ekiwi
ekiwi reviewed Dec 15, 2020
View reviewed changes
src/main/scala/firrtl/passes/LowerTypes.scala
m: ModuleTarget,
instance: DefInstance,
namespace: Namespace,
instRenames: mutable.ListBuffer[(Instance, Instance)],
Copy link
Contributor

Choose a reason for hiding this comment

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

since you are appending, should this better be an ArrayBuffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ListBuffer has constant time for both prepend and append: https://www.scala-lang.org/api/2.12.11/scala/collection/mutable/ListBuffer.html. I use ListBuffer whenever I want to mutably build up an immutable Seq because ListBuffer.toList is free (copy on write to the underlying buffer).

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I learned something!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's pretty neat. I think once we're on 2.13 I might shift to using VectorBuilder since Vector is much better in 2.13.2 and has better memory characteristics than List.

ekiwi
ekiwi approved these changes Dec 15, 2020
View reviewed changes
Copy link
Contributor

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

LGTM as long as all tests pass.

@jackkoenig
Copy link
Contributor Author

LGTM as long as all tests pass.

They were passing but failed because I made RenameMap.fromInstanceRenames package private which broke a test 🙈 . Fixed now.

@jackkoenig
Improve performance of LowerTypes renaming
b9dde5e 
This is done by having LowerTypes uses two RenameMaps instead of one for
each module. There is one for renaming instance paths, and one for
renaming everything within modules.

Also add some utilities:
* TargetUtils for dealing with InstanceTargets
* RenameMap.fromInstanceRenames
@jackkoenig jackkoenig merged commit 15013df into master Dec 16, 2020
mergify bot pushed a commit that referenced this pull request Dec 16, 2020
@jackkoenig
Improve performance of LowerTypes renaming (#2024)
666594b 
This is done by having LowerTypes uses two RenameMaps instead of one for
each module. There is one for renaming instance paths, and one for
renaming everything within modules.

Also add some utilities:
* TargetUtils for dealing with InstanceTargets
* RenameMap.fromInstanceRenames

(cherry picked from commit 15013df)
@mergify mergify bot mentioned this pull request Dec 16, 2020
Merged
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Dec 16, 2020
mergify bot added a commit that referenced this pull request Dec 17, 2020
@mergify @jackkoenig
Improve performance of LowerTypes renaming (bp #2024) (#2025)
7756f8f 
* Improve performance of LowerTypes renaming (#2024)

This is done by having LowerTypes uses two RenameMaps instead of one for
each module. There is one for renaming instance paths, and one for
renaming everything within modules.

Also add some utilities:
* TargetUtils for dealing with InstanceTargets
* RenameMap.fromInstanceRenames

(cherry picked from commit 15013df)

* Add MiMa waiver

Co-authored-by: Jack Koenig <koenig@sifive.com>
@jackkoenig jackkoenig deleted the speedup-lower-types branch March 26, 2021 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekiwi ekiwi ekiwi approved these changes

Assignees
No one assigned
Labels
Backported This PR has been backported to marked stable branch
Projects
None yet
Milestone
1.4.x
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jackkoenig @ekiwi