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 Property expressions, starting with addition. #3810

Merged
merged 11 commits into from Feb 22, 2024

Conversation

mikeurbach
Copy link
Contributor

@mikeurbach mikeurbach commented Feb 2, 2024

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: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

This allows Properties to be used to build up expressions in terms of input Properties and literals.

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
Copy link
Contributor Author

Opening a draft to share an early cut of this. The first commit shows the one time effort to support Property expressions, and new kinds of Property expressions can be added pretty easily similar to the second commit.

@sequencer
Copy link
Member

Is there any firrtl spec for it? Looks like an essential feature for property propagation

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.

Exciting! Maybe add a sentence to the Property explanations doc?

@mikeurbach
Copy link
Contributor Author

Yes, FIRRTL spec and explanation docs are incoming.

@mikeurbach mikeurbach force-pushed the mikeurbach/property-expressions branch from 78b68a5 to c3a35ed Compare February 20, 2024 23:54
@mikeurbach mikeurbach added the Feature New feature, will be included in release notes label Feb 20, 2024
@mikeurbach mikeurbach added this to the 6.x milestone Feb 20, 2024
@mikeurbach
Copy link
Contributor Author

Ok, I have updated the implementation slightly. The APIs for creating Property expressions now immediately create a temp wire and propassign the expression into it, returning the wire for use in other Property expressions.

I think this is the output we are looking for, but I'm curious for feedback on the internal implementation. I still kept the new PropExprBinding and PropExpr as an internal data structures that we can use instead of creating Nodes internally. Does this make sense given our goals? I've looked at how DefPrim works and is converted, and I'm not sure if we want to treat property expressions like a "command", or not.

@mikeurbach mikeurbach marked this pull request as ready for review February 20, 2024 23:58
@mikeurbach
Copy link
Contributor Author

Separately, I'll write some docs for this new feature.

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 don't like putting information that traditionally goes in _ref inside of the Binding, let's meet to chat about how we should do this.

core/src/main/scala/chisel3/internal/firrtl/IR.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Binding.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/properties/Property.scala Outdated Show resolved Hide resolved
@mikeurbach
Copy link
Contributor Author

mikeurbach commented Feb 21, 2024

I'm not sure if this is exactly what you meant, but I realized what you're saying about the binding being redundant. In fe3224b I removed the binding and changed binOp to create the normal binding and immediately setRef to a fresh PropExpr. This works and emits the same.

I guess my broader question is still whether it makes sense to have a separate IR node for PropExpr, which is an Arg but doesn't ever get named. I think it makes sense if we don't want to treat Property expressions as a command that defines a Node. But I'm still kind of curious if this is the best way to factor this if we don't want to create Nodes in the textual FIRRTL for Property expressions.

@mikeurbach mikeurbach force-pushed the mikeurbach/property-expressions branch from 31016b4 to 1e04e27 Compare February 21, 2024 15:20
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, please see comment about sealing PropertyArithmeticOps and other suggestions.

firrtl/src/main/scala/firrtl/ir/IR.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/properties/PropertySpec.scala Outdated Show resolved Hide resolved
@mikeurbach mikeurbach merged commit 11844a4 into main Feb 22, 2024
14 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/property-expressions branch February 22, 2024 14:33
@mergify mergify bot added the Backported This PR has been backported label Feb 22, 2024
mergify bot pushed a commit that referenced this pull request Feb 22, 2024
This allows Properties to be used to build up expressions in terms of input Properties and literals.

(cherry picked from commit 11844a4)
chiselbot pushed a commit that referenced this pull request Feb 22, 2024
This allows Properties to be used to build up expressions in terms of input Properties and literals.

(cherry picked from commit 11844a4)

Co-authored-by: Mike Urbach <mike.urbach@sifive.com>
sequencer pushed a commit that referenced this pull request Feb 28, 2024
This allows Properties to be used to build up expressions in terms of input Properties and literals.
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.

None yet

5 participants