Add Connectable operators: :<=, :>=, :<>=, and :#= #2806
Conversation
Added documentation to :=, <> Renamed ir.BulkConnect to ir.PartialConnect Cleaned up some internal functions
…t), added check on := to barf on mixed bundles
Added documentation to :=, <> Renamed ir.BulkConnect to ir.PartialConnect Cleaned up some internal functions
docs/src/explanations/connectable.md
Outdated
| @@ -284,7 +285,7 @@ The port-direction computation always computes alignment relative to the compone | |||
|
|
|||
| The connection-direction computation always computes alignment based on the explicit consumer/producer referenced for the connection. If one connects `incoming :<>= outgoing`, alignments are computed based on `incoming` and `outgoing`. If one connects `incoming.alignedChild :<>= outgoing.alignedChild`, then alignments are computed based on `incoming.alignedChild` and `outgoing.alignedChild` (and the alignment of `incoming` to `incoming.alignedChild` is irrelevant). | |||
|
|
|||
| This means that users can try to assign to input ports of their module! If I write `x :<>= y`, and `x` is an input to the current module, then that is what the connection is trying to do. However, because input ports are not assignable from within the current module, Chisel will throw an error. This is the same error a user would get using a mono-directioned operator: `x := y` will throw the same error if `x` is an input to the current module. *Whether a component is assignable is irrelevant to the semantics of any connection operator assigning to it.* | |||
| This means that users can try to connect to input ports of their module! If I write `x :<>= y`, and `x` is an input to the current module, then that is what the connection is trying to do. However, because input ports are not connectable from within the current module, Chisel will throw an error. This is the same error a user would get using a mono-directioned operator: `x := y` will throw the same error if `x` is an input to the current module. *Whether a component is connectable is irrelevant to the semantics of any connection operator connecting to it.* | |||
There was a problem hiding this comment.
I think we may have gone a bit far with the connectable adjective here -- does connectable mean driveable? Assignable? Because to me 'connect' is not a direction thing and this whole paragraph doesn't really make sense any more
There was a problem hiding this comment.
I agree, we should use different terminology here like "assignable", "drivable", or "writeable".
We probably need to do hash out all of our terminology and then audit our docs to make sure they're consistent.
| getRecursiveFields.noPath(this).collect { | ||
| case d if d.direction != this.direction => | ||
| Builder.error(s"$this cannot be used with := because submember $d has inverse orientation; use :#= instead") | ||
| } | ||
| getRecursiveFields.noPath(that).collect { | ||
| case d if d.direction != that.direction => | ||
| Builder.error(s"$that cannot be used with := because submember $d has inverse orientation; use :#= instead") | ||
| } |
There was a problem hiding this comment.
Are these recursive checks necessary? Is it not the case that this.direction will be ActualDirection.BidirectionalDirection if there are fields with differing directions? Put another way, I'm pretty sure that this.direction is derived from the fields.
These checks concern me because they are fairly expensive and at bare minimum should use .lazyNoPath to avoid a bunch of Seq allocations and appending to Lists.
There was a problem hiding this comment.
This is only run if you are migrating the mono connect, so I think its ok for performance reasons?
There was a problem hiding this comment.
I don't want to rely on 'ActualDirection' for anything.. maybe I'm just paranoid..
docs/src/explanations/connectable.md
Outdated
| @@ -284,7 +285,7 @@ The port-direction computation always computes alignment relative to the compone | |||
|
|
|||
| The connection-direction computation always computes alignment based on the explicit consumer/producer referenced for the connection. If one connects `incoming :<>= outgoing`, alignments are computed based on `incoming` and `outgoing`. If one connects `incoming.alignedChild :<>= outgoing.alignedChild`, then alignments are computed based on `incoming.alignedChild` and `outgoing.alignedChild` (and the alignment of `incoming` to `incoming.alignedChild` is irrelevant). | |||
|
|
|||
| This means that users can try to assign to input ports of their module! If I write `x :<>= y`, and `x` is an input to the current module, then that is what the connection is trying to do. However, because input ports are not assignable from within the current module, Chisel will throw an error. This is the same error a user would get using a mono-directioned operator: `x := y` will throw the same error if `x` is an input to the current module. *Whether a component is assignable is irrelevant to the semantics of any connection operator assigning to it.* | |||
| This means that users can try to connect to input ports of their module! If I write `x :<>= y`, and `x` is an input to the current module, then that is what the connection is trying to do. However, because input ports are not connectable from within the current module, Chisel will throw an error. This is the same error a user would get using a mono-directioned operator: `x := y` will throw the same error if `x` is an input to the current module. *Whether a component is connectable is irrelevant to the semantics of any connection operator connecting to it.* | |||
There was a problem hiding this comment.
I agree, we should use different terminology here like "assignable", "drivable", or "writeable".
We probably need to do hash out all of our terminology and then audit our docs to make sure they're consistent.
|
Note that I left so many comments, many are hidden. |
|
|
||
| class Example9 extends RawModule { | ||
| val abType = new Record with AutoCloneType { val elements = SeqMap("a" -> Bool(), "b" -> Flipped(Bool())) } | ||
| val bcType = new Record with AutoCloneType { val elements = SeqMap("b" -> Flipped(Bool()), "c" -> Bool()) } |
There was a problem hiding this comment.
| class Example9 extends RawModule { | |
| val abType = new Record with AutoCloneType { val elements = SeqMap("a" -> Bool(), "b" -> Flipped(Bool())) } | |
| val bcType = new Record with AutoCloneType { val elements = SeqMap("b" -> Flipped(Bool()), "c" -> Bool()) } | |
| class Example9 extends RawModule { | |
| val abType = new Record with AutoCloneType { val elements = SeqMap( | |
| "a" -> Bool(), | |
| "b" -> Flipped(Bool() | |
| )) } | |
| val bcType = new Record with AutoCloneType { val elements = SeqMap( | |
| "b" -> Flipped(Bool()), | |
| "c" -> Bool() | |
| ) } |
perhaps?
There was a problem hiding this comment.
(or, save this for later -- did you know we can run scalafmt on our .md files too...
Background
In general, Chisel's connection operators are very broken; their semantics change depending on
import Chisel._vsimport chisel3._. Evenchisel3._'s semantics are not clean; you cannot reason about what will be connected to what unless you know whether the things you are connecting are Wires or IOs. Part of this stems from the fact that we got rid of relative-flippedness for declaring things, instead relying on using Input/Output.This PR defines four new operators which have very clean semantics which depend solely on the hardware type of the things you are connecting. Using them, we plan to migrate all existing operators semantics to compositions of them, but for now we define them independently of Chisel/chisel3, and thus they have more verbose syntax to make them compatible with both imports.
As we continue, we may rename them etc., but their semantics are composable which is an important property. In fact, there are only actually two operators which, when composed in different ways, can implement the semantics of other kinds of operators:
:<=and:>=. While these operators may not be the most common ones to use, understanding them will help users reason about how all the connection operators work.I've written a lot of ScalaDoc and mdoc to describe their semantics.
TODO:
:=to:#=and changes to test, as migration flag is offCurrent requested reviewer feedback
Contributor Checklist
docs/src?Type of Improvement
API Impact
No impact on current code. Adds new operators and documentation.
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.4.x, [small] API extension:3.5.x, API modification or big change:3.6.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.