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

Emit FIRRTL bulkconnects whenever possible #2381

Merged
merged 39 commits into from
Mar 9, 2022

Conversation

jared-barocsi
Copy link
Contributor

This PR aims to emit a FIRRTL bulkconnect when connecting Aggregates either mono- or bi-directionally. This does not cover all possible cases (e.g. ExpandConnects + Dedup), but instead serves as a baseline for most cases when a bulk connect would work.

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?
  • [x 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

  • bug fix
  • performance improvement

API Impact

No impact

Backend Code Generation Impact

No impact

Desired Merge Strategy

Squash and merge

Release Notes

Emit FIRRTL bulk connects whenever possible when using a MonoConnect or BiConnect

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 mark as Please Merge?

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.

This is very exciting! Nice work! I have some suggestions for code improvements as well as 1 suggested case that I suspect isn't currently supported.

core/src/main/scala/chisel3/internal/BiConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/BiConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/BiConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/BiConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved
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. Great work! Thanks!

@jackkoenig jackkoenig merged commit 3553a15 into chipsalliance:master Mar 9, 2022
@jackkoenig
Copy link
Contributor

I meant to mark this for backporting but forgot.

@jackkoenig
Copy link
Contributor

@Mergifyio backport 3.5.x

mergify bot pushed a commit that referenced this pull request Mar 9, 2022
Chisel <> semantics differ somewhat from FIRRTL <= semantics,
so we only emit <= when it would be legal. Otherwise we continue
the old behavior of emitting a connection for every leaf-level
Element.

Co-authored-by: Deborah Soung <debs@sifive.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 3553a15)
@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2022

backport 3.5.x

✅ Backports have been created

mergify bot added a commit that referenced this pull request Mar 10, 2022
Chisel <> semantics differ somewhat from FIRRTL <= semantics,
so we only emit <= when it would be legal. Otherwise we continue
the old behavior of emitting a connection for every leaf-level
Element.

Co-authored-by: Deborah Soung <debs@sifive.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 3553a15)

Co-authored-by: Jared Barocsi <82000041+jared-barocsi@users.noreply.github.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>
jackkoenig added a commit that referenced this pull request Feb 28, 2023
Reset, AsyncReset, Interval, attach, assert, assume, and cover have all
been added as keywords but not added to the allowlist for parsing as
ids.
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.

4 participants