Skip to content

Support BoringUtils.bore on OpaqueType wrapping a Property. #4337

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
Merged
merged 3 commits into from
Aug 9, 2024
Merged

Support BoringUtils.bore on OpaqueType wrapping a Property. #4337

merged 3 commits into from
Aug 9, 2024

Conversation

mikeurbach
Copy link
Contributor

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 request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

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

Release Notes

This ensures boring from an OpaqueType that wraps a Property uses the correct connection operator in the IR.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.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.

@mikeurbach mikeurbach added the Internal Internal change, does not affect users, will be included in release notes label Aug 9, 2024
@mikeurbach mikeurbach requested a review from jackkoenig August 9, 2024 20:22
mikeurbach
mikeurbach commented Aug 9, 2024
View reviewed changes
core/src/main/scala/chisel3/RawModule.scala Outdated
@@ -239,6 +239,10 @@ abstract class RawModule extends BaseModule {
case (false, false) =>
// For non-probe, directly create Nodes for lhs, skipping visibility check to support BoringUtils.drive.
(left, right) match {
case (lhsOpaque: OpaqueType, rhsOpaque: OpaqueType)
if DataMirror
.isProperty(lhsOpaque.allElements.head) && DataMirror.isProperty(rhsOpaque.allElements.head) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if there is a better way to query the underlying Data than allElements.head... if this is about the best we can do, maybe I can add an internal-only helper method to access this?

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment on all 4 lines with suggestion.

jackkoenig
jackkoenig reviewed Aug 9, 2024
View reviewed changes
core/src/main/scala/chisel3/RawModule.scala Outdated
Comment on lines 242 to 245
case (lhsOpaque: OpaqueType, rhsOpaque: OpaqueType)
if DataMirror
.isProperty(lhsOpaque.allElements.head) && DataMirror.isProperty(rhsOpaque.allElements.head) =>
PropAssign(si, Node(lhsOpaque.allElements.head), Node(rhsOpaque.allElements.head))
Copy link
Contributor

@jackkoenig jackkoenig Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
case (lhsOpaque: OpaqueType, rhsOpaque: OpaqueType)
if DataMirror
.isProperty(lhsOpaque.allElements.head) && DataMirror.isProperty(rhsOpaque.allElements.head) =>
PropAssign(si, Node(lhsOpaque.allElements.head), Node(rhsOpaque.allElements.head))
case (lhsOpaque: Record, rhsOpaque: Record)
if lhsOpaque._isOpaqueType && rhsOpaque._isOpaqueType && DataMirror.isProperty=>
secretConnection(lhsOpaque.getElements.head, rhsOpaque.getElements.head)

OpaqueType the trait isn't enough to check (and note the trait is self-typed by Record), instead we match on Record and check _isOpaqueType. Also this needs to be recursive in some way because you can have OpaqueTypes of OpaqueTypes. It's turtles all the way down!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. What if DataMirror.isProperty handles _isOpaqueType and recursively checking? Then we could have one case here that just uses the if DataMirror.isProperty condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh I guess it's not that easy, this needs to recursively pick off the allElements.head till it gets to the bottom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think fa90b8f should handle it, this is now using _isOpaqueType and recursively unwrapping OpaqueTypes.

jackkoenig
jackkoenig approved these changes Aug 9, 2024
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, see suggestions though to fix the compile issue.

jackkoenig
jackkoenig reviewed Aug 9, 2024
View reviewed changes
@mikeurbach
Copy link
Contributor Author

Oh, thanks for fixing the si shadowing @jackkoenig!

@jackkoenig jackkoenig added this to the 6.x milestone Aug 9, 2024
@jackkoenig jackkoenig added Bugfix Fixes a bug, will be included in release notes and removed Internal Internal change, does not affect users, will be included in release notes labels Aug 9, 2024
@jackkoenig jackkoenig merged commit 5dd085c into main Aug 9, 2024
17 checks passed
@jackkoenig jackkoenig deleted the mikeurbach/boring-utils-opaque-property branch August 9, 2024 22:35
@mergify mergify bot added the Backported This PR has been backported label Aug 9, 2024
mergify bot pushed a commit that referenced this pull request Aug 9, 2024
@mikeurbach @mergify
Support BoringUtils.bore on OpaqueType wrapping a Property. (#4337)
318f52a 
Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 5dd085c)

# Conflicts:
#	core/src/main/scala/chisel3/RawModule.scala
#	src/test/scala/chiselTests/BoringUtilsSpec.scala
@mergify mergify bot mentioned this pull request Aug 9, 2024
Merged
14 tasks
jackkoenig added a commit that referenced this pull request Nov 25, 2024
@mikeurbach @jackkoenig
Support BoringUtils.bore on OpaqueType wrapping a Property. (#4337)
2631954 
Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 5dd085c)
chiselbot pushed a commit that referenced this pull request Nov 25, 2024
@mergify @mikeurbach
Support BoringUtils.bore on OpaqueType wrapping a Property. (#4337) (#…
61c4893 
…4338)

Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 5dd085c)

Co-authored-by: Mike Urbach <mike.urbach@sifive.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackkoenig jackkoenig jackkoenig approved these changes

Assignees
No one assigned
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes
Projects
None yet
Milestone
6.x
Development

Successfully merging this pull request may close these issues.
2 participants
@mikeurbach @jackkoenig