Support probe-define in monoconnects #3566

rwy7 · 2023-10-03T22:45:21Z

Enable mono/ :=connects for probes, which will emit a ProbeDefine.
Also enables the fancier connect forms
A "Probe" will be connected at the root

Co-authored-by: Will Dietz will.dietz@sifive.com

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)
API modification

Desired Merge Strategy

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

Release Notes

Enable mono/ :=connects for probes, which will emit a ProbeDefine.
Also enables the fancier connect forms
A "Probe" will be connected at the root

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.

src/test/scala/chiselTests/ProbeSpec.scala

jackkoenig · 2023-10-04T20:15:12Z

core/src/main/scala/chisel3/reflect/DataMirror.scala

-        case a: Record =>
+        case a: Record if (!hasProbeTypeModifier(a)) =>
          a._elements.iterator.flatMap {
            case (fieldName, fieldData) =>
              collectMembersAndPaths(fieldData, s"$path.$fieldName")(collector)
          }
-        case a: Vec[_] =>
+        case a: Vec[_] if (!hasProbeTypeModifier(a)) =>
          a.elementsIterator.zipWithIndex.flatMap {
            case (fieldData, fieldIndex) =>
              collectMembersAndPaths(fieldData, s"$path($fieldIndex)")(collector)


Why are these here? It seems to me that DataMirror should return probes as well and perhaps the onus is on the user providing a collector function that excludes probes if that is the behavior they want?

This stops it from recursing /through/ probes, not excluding them, FWIW.

The collector you're describing is used in, e.g., excludeProbes, but the idea here is that Probes are opaque in the sense that's being iterated over here -- they're basically Element in that sense, with an underlying probed type that itself may have children but the probe itself does not.

As-is no collector definition (AFAIK?) can use this (pre-this-PR) to provide the useful behavior of treating probes as the leaves of aggregates that they are.

All that said, what this method is """supposed""" to do independent of our immediate needs I can't speak for (such as it's expected to recurse in same way other things do), so if it's not directly needed sure we can drop it, if that seems better!

This stops it from recursing /through/ probes, not excluding them, FWIW.

Good point, thanks for pointing that out

The collector you're describing is used in, e.g., excludeProbes, but the idea here is that Probes are opaque in the sense that's being iterated over here -- they're basically Element in that sense, with an underlying probed type that itself may have children but the probe itself does not.

As-is no collector definition (AFAIK?) can use this (pre-this-PR) to provide the useful behavior of treating probes as the leaves of aggregates that they are.

This is a fair point, but I think it highlights a much larger issue (and one that we do currently run into with OpaqueTypes as well) that we don't really have a way to talk about "leaves" other than Element. I think the change you're getting at here makes a lot of sense, but I think it necessitates coming up with the new API for "leaf".

All that said, what this method is """supposed""" to do independent of our immediate needs I can't speak for (such as it's expected to recurse in same way other things do), so if it's not directly needed sure we can drop it, if that seems better!

You've convinced me that this is the right thing to do for this PR. We probably need to prioritize finding a better way to discuss leaves though.

I want to note that collectLeafMembers above is probably broken as it is expressed as:

def collectLeafMembers(d: Data): Seq[Data] = DataMirror.collectMembers(d) { case x: Element => x }.toVector

It will ignore Aggregate probes even though those are leaves. It's also extremely likely that anyone calling that method is immediately matching on Element though....

💯 , agreed and interesting/good to know about OpaqueType. Hopefully soon we can get into business of working on these things and making everything awesome 🚀 .

Good catch re:collectLeafMembers, thank you for looking / thinking about that!! I think you're right.

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

dtzSiFive · 2023-10-04T13:04:07Z

core/src/main/scala/chisel3/connectable/Connection.scala

@@ -184,20 +184,21 @@ private[chisel3] object Connection {
        // Recursive Case 4: non-empty orientations
        case (conAlign: NonEmptyAlignment, proAlign: NonEmptyAlignment) =>
          (conAlign.member, proAlign.member) match {
-            case (consumer: Aggregate, producer: Aggregate) =>
+            case (consumer: Aggregate, producer: Aggregate)


It'd be great to make Probes their own type (since they are), but for now these guards help treat probes in this way.

dtzSiFive · 2023-10-04T13:07:19Z

src/test/scala/chiselTests/ProbeSpec.scala

          })
          io.out :<>= io.in
        },
        Array("--throw-on-first-error")
      )
    }
-    exc.getMessage should include("Cannot use connectables with probe types. Exclude them prior to connection.")
+    exc.getMessage should include(
+      "mismatched probe/non-probe types in ProbeSpec_Anon.io.out[0]: IO[Bool] and ProbeSpec_Anon.io.in[0]: IO[Bool]."


(Pre-existing issue/opportunity for improvement: Probe's aren't printed accurately, printed ignoring their probe-ness)

dtzSiFive · 2023-10-04T20:49:56Z

core/src/main/scala/chisel3/reflect/DataMirror.scala

-        case a: Record =>
+        case a: Record if (!hasProbeTypeModifier(a)) =>
          a._elements.iterator.flatMap {
            case (fieldName, fieldData) =>
              collectMembersAndPaths(fieldData, s"$path.$fieldName")(collector)
          }
-        case a: Vec[_] =>
+        case a: Vec[_] if (!hasProbeTypeModifier(a)) =>
          a.elementsIterator.zipWithIndex.flatMap {
            case (fieldData, fieldIndex) =>
              collectMembersAndPaths(fieldData, s"$path($fieldIndex)")(collector)


This stops it from recursing /through/ probes, not excluding them, FWIW.

The collector you're describing is used in, e.g., excludeProbes, but the idea here is that Probes are opaque in the sense that's being iterated over here -- they're basically Element in that sense, with an underlying probed type that itself may have children but the probe itself does not.

As-is no collector definition (AFAIK?) can use this (pre-this-PR) to provide the useful behavior of treating probes as the leaves of aggregates that they are.

All that said, what this method is """supposed""" to do independent of our immediate needs I can't speak for (such as it's expected to recurse in same way other things do), so if it's not directly needed sure we can drop it, if that seems better!

mwachs5

maybe we should add some words to this to explain how probe-ness has to match too: https://github.com/chipsalliance/chisel/blob/main/docs/src/explanations/connectable.md#terminology

In the "structural type check" bullet are we now saying that "structural type check" includes 'probe-ness', or "alignment type check" includes 'probe-ness', or should "probe-ness type check" be its own bullet

rwy7 · 2023-10-05T14:09:32Z

docs/src/explanations/connectable.md

@@ -43,7 +43,7 @@ section: "chisel3"
   * E.g. `IO(Decoupled(Bool)).ready` is a member of the parent `IO` component
 * "relative alignment" - whether two members of the same component or Chisel type are aligned/flipped, relative to one another
   * see section [below](#alignment-flipped-vs-aligned) for a detailed definition
- * "structural type check" - Chisel type `A` is structurally equivalent to Chisel type `B` if `A` and `B` have matching bundle field names and types (`Record` vs `Vector` vs `Element`), vector sizes, `Element` types (UInt/SInt/Bool/Clock etc))
+ * "structural type check" - Chisel type `A` is structurally equivalent to Chisel type `B` if `A` and `B` have matching bundle field names and types (`Record` vs `Vector` vs `Element`), probe modifiers (probe vs nonprobe), vector sizes, `Element` types (UInt/SInt/Bool/Clock)


OK, I've snuck in a bit that says, A and B should have matching probe modifiers.

dtzSiFive

LGTM, thanks!

dtzSiFive · 2023-10-16T22:36:45Z

(6.0 milestone sound right?)

jackkoenig · 2023-10-17T15:57:13Z

(6.0 milestone sound right?)

Yep, Probes are new in Chisel 6

rwy7 and others added 10 commits October 3, 2023 15:23

WIP: Support probe define in monoconnects and other fancier connect ops

80712bc

touchups

5e555af

ProbeSpec: fix, test connecting agg-of-probe

ed186bb

fix, test a bit

d9d9063

more fix, test turkducken

38a294a

scalafmt

f0cce67

more test

16ac3f8

matchingChildren: ... probes

3f88a7f

doConnection: Don't recurse into probes.

21b1432

Sbt fmt

2e078be

rwy7 marked this pull request as ready for review October 4, 2023 13:56

seldridge added the Feature New feature, will be included in release notes label Oct 4, 2023

mwachs5 reviewed Oct 4, 2023

View reviewed changes

src/test/scala/chiselTests/ProbeSpec.scala Show resolved Hide resolved

jackkoenig reviewed Oct 4, 2023

View reviewed changes

src/test/scala/chiselTests/ProbeSpec.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Oct 4, 2023

View reviewed changes

rwy7 and others added 2 commits October 4, 2023 16:16

Flow check connections between probes

46e4acd

Update src/test/scala/chiselTests/ProbeSpec.scala

edd4660

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

dtzSiFive reviewed Oct 4, 2023

View reviewed changes

mwachs5 reviewed Oct 4, 2023

View reviewed changes

rwy7 added 2 commits October 4, 2023 21:45

collectLeafElements should also return probes

fc51f56

Mention probe-modifier in definition of structural type check

3e3c75e

rwy7 force-pushed the rwy-probes branch from 35877bd to 3e3c75e Compare October 5, 2023 14:08

rwy7 commented Oct 5, 2023

View reviewed changes

dtzSiFive approved these changes Oct 16, 2023

View reviewed changes

jackkoenig merged commit 54fbf40 into chipsalliance:main Oct 17, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support probe-define in monoconnects #3566

Support probe-define in monoconnects #3566

rwy7 commented Oct 3, 2023 •

edited

jackkoenig Oct 4, 2023

dtzSiFive Oct 4, 2023

jackkoenig Oct 4, 2023

dtzSiFive Oct 4, 2023

dtzSiFive Oct 4, 2023

dtzSiFive Oct 4, 2023

dtzSiFive Oct 4, 2023

mwachs5 left a comment

rwy7 Oct 5, 2023

dtzSiFive left a comment

dtzSiFive commented Oct 16, 2023

jackkoenig commented Oct 17, 2023

Support probe-define in monoconnects #3566

Support probe-define in monoconnects #3566

Conversation

rwy7 commented Oct 3, 2023 • edited

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

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

mwachs5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzSiFive left a comment

Choose a reason for hiding this comment

dtzSiFive commented Oct 16, 2023

jackkoenig commented Oct 17, 2023

rwy7 commented Oct 3, 2023 •

edited