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

Add support for marking things as readOnly #4190

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Add support for marking things as readOnly #4190

merged 1 commit into from
Jun 20, 2024

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Jun 18, 2024

Also add internal support for marking things as "deprecated read only". This will promptly be used to mark the return value of .asTypeOf as "deprecated read only" in 6.x in preparation of #4168.

I implemented this as part of the view binding for the following reasons:

  1. While one could mutate the underlying Data, there are use cases where an API may want to return a read-only reference/view of their underlying Data while reserving the right to connect to it themselves (I intend to do this in proprietary code)
  2. We could implement this as a separate Binding (i.e. not part of the existing Views) or as an Arg. Either approach could certainly work, but it would need to compose with views anyway. Consider some of the tests. Given some Data that is marked readOnly that is then combined with other (potentially not read-only) Data to be viewed as some Aggregate type--the read-only aspect of that target needs to be preserved. This "writeability" is thus an intrinsic property of reification anyway, so we might as well just embrace it and make it part of the API so that callers handle it as appropriate (which is what I implemented).

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

  • Feature (or new API)

Desired Merge Strategy

  • Squash

Release Notes

Users can call .readOnly on any Data to prevent connections to the returned value. Resolves #1267.

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.

@jackkoenig jackkoenig added the Feature New feature, will be included in release notes label Jun 18, 2024
@jackkoenig jackkoenig added this to the 6.x milestone Jun 18, 2024
@jackkoenig
Copy link
Contributor Author

I'm going to peel off the bit that actually marks the return value as "deprecated read-only" into its own PR.

case (Output, Output) => issueConnectR2L(left, right)
case (Internal, Output) => issueConnectR2L(left, right)
case (Output, Output) => issueConnectR2L(left, lwr, right, rwr)
case (Internal, Output) => issueConnectR2L(left, lwr, right, rwr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff here can probably be made lower touch. The issueConnect functions call .lref so we could probably rely on the error reporting there, although that means it would construct wires in the error case (to provide a legal lref for a read-only thing) whereas doing it at this level allows us to elide issuing the connection altogether.

Separate from that, this code is also pretty inelegant in that it's checking .topBinding for each left and right multiple times. This may be optimized by the JIT, but it's pretty inefficient if it isn't.

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

This approach sounds good to me. I don't have time to do a thorough review today, so please bug me for a more detailed post-merge review.

Also add internal support for marking things as "deprecated read only".
@jackkoenig jackkoenig enabled auto-merge (squash) June 20, 2024 21:37
@jackkoenig jackkoenig merged commit 7be4243 into main Jun 20, 2024
14 checks passed
@jackkoenig jackkoenig deleted the readonly branch June 20, 2024 21:47
@mergify mergify bot added the Backported This PR has been backported label Jun 20, 2024
mergify bot pushed a commit that referenced this pull request Jun 20, 2024
Also add internal support for marking things as "deprecated read only".

(cherry picked from commit 7be4243)

# Conflicts:
#	core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala
#	core/src/main/scala/chisel3/internal/Binding.scala
chiselbot pushed a commit that referenced this pull request Jun 20, 2024
* Add support for marking things as readOnly (#4190)

Also add internal support for marking things as "deprecated read only".

(cherry picked from commit 7be4243)

# Conflicts:
#	core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala
#	core/src/main/scala/chisel3/internal/Binding.scala

* Resolve backport conflicts

---------

Co-authored-by: Jack Koenig <koenig@sifive.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API to turn Data into read-only reference
2 participants