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

Move more Data methods to BaseType and base connection checks on BaseType. #3442

Merged
merged 1 commit into from Aug 3, 2023
Merged

Move more Data methods to BaseType and base connection checks on BaseType. #3442

merged 1 commit into from Aug 3, 2023

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

MonoConnect.elemConnect is updated to accept any BaseType, so its checks can be re-used for any source and sink BaseType that are already known to be compatible.

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.5.x, 3.6.x, or 5.x depending on impact, API modification or big change: 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.

@mikeurbach
Copy link
Contributor Author

@jackkoenig @azidar @mwachs5 I don't have the ability to request reviews yet, but this is the next (and last, for now) internal refactoring that will help Property types re-use some existing functionality.

mwachs5
mwachs5 reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

Seems like at the point where we are actually issuing the connect it should error out if it doesn't know what to emit, and at this point we only know how to emit for Data, right?

core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved
@mikeurbach mikeurbach force-pushed the mikeurbach/monoconnect-isbindable branch from a0a0fce to 7063aa0 Compare July 27, 2023 19:03
mwachs5
mwachs5 previously approved these changes Jul 28, 2023
View reviewed changes
@mwachs5 mwachs5 mentioned this pull request Jul 28, 2023
Merged
14 tasks
@mikeurbach mikeurbach force-pushed the mikeurbach/monoconnect-isbindable branch 4 times, most recently from 12702a5 to f1ca2b9 Compare July 31, 2023 15:45
@mikeurbach
Copy link
Contributor Author

@jackkoenig I think this is ready for another look when you get a chance. Curious what you think of the refactor I did after the thread last week.

@jackkoenig jackkoenig added the Internal Internal change, does not affect users, will be included in release notes label Aug 1, 2023
@jackkoenig jackkoenig dismissed mwachs5’s stale review August 1, 2023 23:47

Implementation has changed quite a bit.

jackkoenig
jackkoenig reviewed Aug 1, 2023
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.

I approve of the changes to MonoConnect modulo a few nits, but that's very clever, nice.

I have some questions about ref (and perhaps we should also think about them for lref).

core/src/main/scala/chisel3/Data.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/MonoConnect.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/Data.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/Data.scala Outdated Show resolved Hide resolved
@mikeurbach mikeurbach force-pushed the mikeurbach/monoconnect-isbindable branch from f1ca2b9 to 0817e22 Compare August 2, 2023 16:17
@mikeurbach
Copy link
Contributor Author

mikeurbach commented Aug 2, 2023 •
edited

In the last update I dialed back most of the changes to move methods from Data to BaseType. Now it is just name-related helpers for the error messages used in checkConnection. I think a lot of the comments just go away since I'm not moving those. I think I've also addressed the antipatterns in the new methods in MonoConnect.scala. So this should be ready for another review.

@mikeurbach mikeurbach changed the title Move more Data methods to BaseType and base MonoConnect.elemConnect on BaseType. Move more Data methods to BaseType and base connection checks on BaseType. Aug 2, 2023
@mikeurbach
Copy link
Contributor Author

@jackkoenig when you get a chance, please take one last look, but I think this should be ready to merge.

jackkoenig
jackkoenig approved these changes Aug 3, 2023
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.

Nice work! Thanks for humoring me 🙂

@jackkoenig jackkoenig added this to the 6.0 milestone Aug 3, 2023
@jackkoenig jackkoenig merged commit bb73d1a into chipsalliance:main Aug 3, 2023
14 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/monoconnect-isbindable branch August 3, 2023 00:07
@mikeurbach mikeurbach mentioned this pull request Aug 10, 2023
Merged
14 tasks
mikeurbach added a commit that referenced this pull request Aug 11, 2023
@mikeurbach
Support connecting Properties. (#3481)
ec75dec 
This includes the API, Chisel IR, Converter, FIRRTL IR, and Serializer
support for Property connections.

It also includes moving requireVisible from Data to BaseType, which is
required but was omitted from #3442.
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

@mwachs5 mwachs5 mwachs5 left review comments

Assignees
No one assigned
Labels
Internal Internal change, does not affect users, will be included in release notes
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mikeurbach @jackkoenig @mwachs5