Initial properties package and Property type. #3425

mikeurbach · 2023-07-14T16:03:29Z

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

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 · 2023-07-14T16:04:32Z

This just sets the stage for properties. I'll send a PR next that will show the first Property types.

Should I add a page under src/docs for properties?

mikeurbach · 2023-07-14T16:07:31Z

I filed a PR against this branch on my fork to show what the first properties additions look like: mikeurbach#1

core/src/main/scala/chisel3/properties/Property.scala

mwachs5 · 2023-07-17T16:17:00Z

Should I add a page under src/docs for properties?

Yes, I think it would be good to put something under the explanations or cookbooks for them, depending on what kind of doc you plan to write.

When adding something there as a new page there are 3 things to update:

write your new page
add it in the ToC for that section, e.g if it's for explanations add it here, note it is in the order that you think a person should read it: https://github.com/chipsalliance/chisel/blob/main/docs/src/explanations/explanations.md
add it to the website sidebar in the same order as 2 here, again if you are doing explanation then it would go in the corresponding slot here: https://github.com/chipsalliance/chisel/blob/main/website/docs/src/main/resources/microsite/data/menu.yml#L42

mikeurbach · 2023-07-19T22:34:44Z

This now includes #3429. I'd base this PR on that one for initial review, but this PR is on upstream chisel not my fork. I can open a PR within my fork for early review here if it would be helpful.

jackkoenig · 2023-07-21T00:07:07Z

core/src/main/scala/chisel3/properties/Property.scala

+private[chisel3] class PropertyType[T]
+
+/** Companion object for PropertyType.
+  *
+  * Typeclass instances for valid Property types are defined here, so they will
+  * be in the implicit scope and available for users.
+  */
+private[chisel3] object PropertyType {}


I don't know that these can really be package private and yet used in a public interface (on Property below), I think we will need to make these public.

In a later PR, where I actually add some typeclass instances here, it seems to work fine from the unit tests. Maybe I'm missing something--is there something I should test that would require this object to be public?

Perhaps it's changed in the last few days, but I don't see any unit tests calling this, only 1 that checks that it fails to compile. I think if you actually try to use this code from outside of the chisel3 package, you will get compile errors.

This draft PR on my fork is no longer in sync, but I think it indicates what I mean: https://github.com/mikeurbach/chisel3/pull/1/files#diff-4b64ebc257d840c404c27e41af276a1ae760940bdf80acf7f0cac225100fc944R20

There are tests in the chiselTests package that successfully create properties, and PropertyType is still package private. Maybe I'm missing something, but it seems like just Property needs to be public, and Scala implicit resolution did the rest.

mikeurbach · 2023-07-27T16:30:06Z

I rebased this on main and added a little section in the docs. I expect this will be fleshed out in future PRs as new property types are actually added. I think this is ready for a round of review @jackkoenig @mwachs5 .

mwachs5 · 2023-07-28T18:27:56Z

core/src/main/scala/chisel3/properties/package.scala

+  * This entire package is currently very experimental, so expect some rough
+  * edges and rapid API evolution.
+  */
+package object properties {}


what is this for?

I'm not sure if it's needed... I was just adding this boilerplate in case we want to define any functions in here later.

mwachs5 · 2023-07-28T18:29:21Z

src/test/scala/chiselTests/properties/PropertySpec.scala

+  it should "fail to compile with unsupported Property types" in {
+    assertTypeError("""
+      class MyThing
+      val badProp = Property[MyThing]()


Can you show the failing case where we try to propOut := propIn and you get the error message that its not legal? Such that in a subsequent PR it will be shown to now be legal?

But that same test would show that you can indeed have IO ports of property types?

Well on this PR := is not even defined, so it will fail for that reason. Two PRs down the road there will be an example with both success and failure for :=.

ok. i guess i thought this one was slated to land after #3442

but in this PR i should be able to have IO of Input, Output properties? Can that be shown?

i guess i thought this one was slated to land after

These are currently independent, there will be a future change to actually make properties connectable.

i should be able to have IO of Input, Output properties? Can that be shown?

Not in the current PR, since no actual types are being defined to work with Property (there are no typeclass instances for PropertyType). I'm trying to just stage these in small pieces with this piece being the initial boilerplate. In the next PR, you will be able to have an IO of Input or Output properties. If it would help, I can make draft PRs for the next two changes.

ok, its fine. sounds like you are being cognizant of having the tests come in with the right change

yep trying to break this up into small pieces with tests for what is supported in each piece. This PR is mostly boilerplate, but we can at least test one error.

mwachs5 · 2023-07-28T18:30:23Z

core/src/main/scala/chisel3/properties/Property.scala

+  private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection): Unit = {
+    this.maybeAddToParentIds(target)
+    binding = target
+    val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection)


are you allowed to have Flipped properties?

Hmm I mainly care about properties having Input/Output for their SpecifiedDirection and ActualDirection. When we get to property aggregates, I'm hoping to avoid having flips and have aggregate properties all point the same direction.

mikeurbach · 2023-07-31T19:27:29Z

@jackkoenig @mwachs5 any other feedback on this initial PR? I have been using it in later patches in this series and it's working for me. For a preview, here's a draft PR of the next patch, based on this one: mikeurbach#1

mikeurbach · 2023-08-03T18:23:17Z

I just force pushed this--no changes, but rebasing on main. That makes it easier for me to stack up the next three PRs, now that the required refactors are in main.

trilorez · 2023-08-03T21:42:16Z

core/src/main/scala/chisel3/properties/Property.scala

+
+/** Companion object for PropertyType.
+  *
+  * Typeclass instances for valid Property types are defined here, so they will


Will this approach be flexible enough for user-defined struct properties? It seems like everything needs to be declared here to be supported.
I suppose if the plan is to have some sort of property aggregate base class this would work. Or just only supporting a Scala dictionary for that use case.

Yeah it's a good question. I haven't thought about user-defined struct properties yet. We will have some kinds of aggregates, like a Scala Map as a property. Fairly soon, we will also add class/object to Chisel, which is sort of like a Module but only with Property typed ports. And an instance of a class (an object) will itself have a property type. So for user-defined "structs", I think we can support them that way.

In general, I think we will have to have a known set of types that can be used with Properties, if we want to take care of defining how they serialize to FIRRTL. So hopefully the above is enough extensibility, and we can still keep the set of possible Property types a sealed thing.

core/src/main/scala/chisel3/properties/Property.scala

jackkoenig · 2023-08-08T00:01:02Z

core/src/main/scala/chisel3/properties/Property.scala

+  /** Clone type by simply constructing a new Property[T].
+    */
+  override def cloneType: this.type = new Property[T].asInstanceOf[this.type]
+
+  /** Clone type with extra information preserved.
+    *
+    * The only extra information present on a Property type is directionality.
+    */
+  private[chisel3] override def cloneTypeFull: this.type = {
+    val clone = this.cloneType
+    clone.specifiedDirection = specifiedDirection
+    clone
+  }


If we go the more type-classy route, I'm wondering if .cloneType is something we're actually going to need. I know it's needed for now so not really a comment on this PR, but something we should keep in mind as we continue to flesh this API out.

jackkoenig

I feel like the next PR is going to be the more important one so let's just get this one in and continue fleshing this API out

mikeurbach · 2023-08-08T01:52:49Z

Thanks @jackkoenig !

mikeurbach mentioned this pull request Jul 14, 2023

[minor] Add initial description of property types. chipsalliance/firrtl-spec#121

Merged

mwachs5 reviewed Jul 14, 2023

View reviewed changes

core/src/main/scala/chisel3/properties/Property.scala Show resolved Hide resolved

jackkoenig reviewed Jul 14, 2023

View reviewed changes

core/src/main/scala/chisel3/properties/Property.scala Outdated Show resolved Hide resolved

mikeurbach force-pushed the mikeurbach/properties-initial branch from 78462fd to 44c5432 Compare July 19, 2023 22:19

jackkoenig reviewed Jul 21, 2023

View reviewed changes

mikeurbach force-pushed the mikeurbach/properties-initial branch 7 times, most recently from 139b9e5 to acbe1cb Compare July 27, 2023 16:29

mwachs5 reviewed Jul 28, 2023

View reviewed changes

jackkoenig added the Feature New feature, will be included in release notes label Aug 3, 2023

Initial properties package and Property type.

cdc09cd

mikeurbach force-pushed the mikeurbach/properties-initial branch from acbe1cb to cdc09cd Compare August 3, 2023 18:22

trilorez reviewed Aug 3, 2023

View reviewed changes

jackkoenig reviewed Aug 7, 2023

View reviewed changes

core/src/main/scala/chisel3/properties/Property.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Aug 8, 2023

View reviewed changes

jackkoenig approved these changes Aug 8, 2023

View reviewed changes

Make Property tpe val package private.

a284b18

jackkoenig merged commit da90c5b into chipsalliance:main Aug 8, 2023
15 checks passed

mikeurbach added this to the 6.0 milestone Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial properties package and Property type. #3425

Initial properties package and Property type. #3425

mikeurbach commented Jul 14, 2023

mikeurbach commented Jul 14, 2023

mikeurbach commented Jul 14, 2023

mwachs5 commented Jul 17, 2023

mikeurbach commented Jul 19, 2023

jackkoenig Jul 21, 2023

mikeurbach Jul 27, 2023

jackkoenig Jul 28, 2023

mikeurbach Jul 28, 2023

mikeurbach commented Jul 27, 2023

mwachs5 Jul 28, 2023

mikeurbach Jul 28, 2023

mwachs5 Jul 28, 2023

mwachs5 Jul 28, 2023

mikeurbach Jul 28, 2023

mwachs5 Jul 28, 2023

mwachs5 Jul 28, 2023

mikeurbach Jul 28, 2023

mwachs5 Jul 28, 2023

mikeurbach Jul 28, 2023

mwachs5 Jul 28, 2023

mikeurbach Jul 28, 2023

mikeurbach commented Jul 31, 2023

mikeurbach commented Aug 3, 2023

trilorez Aug 3, 2023

mikeurbach Aug 3, 2023

jackkoenig Aug 8, 2023

jackkoenig left a comment

mikeurbach commented Aug 8, 2023

Initial properties package and Property type. #3425

Initial properties package and Property type. #3425

Conversation

mikeurbach commented Jul 14, 2023

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

mikeurbach commented Jul 14, 2023

mikeurbach commented Jul 14, 2023

mwachs5 commented Jul 17, 2023

mikeurbach commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeurbach commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeurbach commented Jul 31, 2023

mikeurbach commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

mikeurbach commented Aug 8, 2023