Removed flow and replaced with source/sink and relative alignment #139
Conversation
azidar
force-pushed
the
azidar/remove-flow
branch
from
013a706
to
ec45136
Compare
|
Cool. Let me take some time to look this over. As an initial thought: You're using the term "subcomponent". Could we wait until we have the term subcomponent formally defined before merging this in? |
|
It’s glad to see firrtl is eventually removing the output and input with
Alignment!
I wonder is this a breaking API in chisel? Or we can seamlessly migrate out
current API to this?
…On Sat, Nov 4, 2023 at 02:29 Michael Maloney ***@***.***> wrote:
Cool. Let me take some time to look this over.
As an initial thought: You're using the term "subcomponent". Could we wait
until we have the term subcomponent formally defined before merging this in?
—
Reply to this email directly, view it on GitHub
<#139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMF2K7KYMEGFFUB3FTMIR3YCUZZDAVCNFSM6AAAAAA64Z63GOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJSHE2DAMJRG4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
There was a problem hiding this comment.
High level:
- Flipped/Aligned matters narrowly for connection semantics. Can these changes be sunk into the
Connection Algorithmsection and then used to rewrite that section? The motivation for this is that Flipped/Aligned are concepts which are only relevant to dealing with connects. We can keep the spec tighter by only introducing these concepts there. - Can the discussion of relative alignment be simplified to only discuss parent/child alignment once this is sunk? My reasoning here is that relative alignment of siblings I don't think is used and it would be terser to not mention it.
Pulling on (2), I think the essence of the concept is as follows:
A connection sets two things: (a) a base alignment of "aligned" and (b) a base component with a base type specified by the type of either the source or sink expression (choice of source or sink doesn't matter as the types must be equivalent). All leaf sub-components of the source and sink are then connected. Leaf sub-components are connected source-to-sink if the relative alignment of the leaf sub-component and the base component/type are aligned. Leaf sub-components are connected sink-to-source if the relative alignment of the leaf sub-component and base component/type are flipped.
When described as such the notion of sibling alignment doesn't arise.
WDYT?
spec.md
Outdated
| With the following exceptions, all other hardware components/subcomponents can be referenced with a sink expression: | ||
| - input ports (or subcomponents which are input ports) | ||
| - data field of memory read ports | ||
| - rdata field of memory read-write ports |
There was a problem hiding this comment.
Additionally, no primitive operations may be used as sink expressions. Given @mmaloney-sf's new grammar, it should be safe to say that only references are legal sink expressions. Then introduce the restrictions on what references are not legal as sinks.
There was a problem hiding this comment.
Excellent, I'll revisit this when I bump.
Clarify working on source expressions Co-authored-by: Schuyler Eldridge <[email protected]>
| - `p` is aligned w.r.t `p.alignedChild` | ||
| - `p` is flipped w.r.t `p.flippedChild` |
There was a problem hiding this comment.
this is sort of weirdly written. I know it is a symmetrical relationship, but wouldn't it be more intuitive to write it as
p.aligned child is aligned w.r.t. p
| Every sink-flow probe must be the target of exactly one of these statements. | ||
| Every sink probe expression must be the target of exactly one of these statements. | ||
|
|
||
| The define statement takes a sink static reference target and sets it to the specified reference, which must either be a compatible probe expression or static reference source. |
There was a problem hiding this comment.
| The define statement takes a sink static reference target and sets it to the specified reference, which must either be a compatible probe expression or static reference source. | |
| The `define`{.firrtl} statement takes a sink static reference target and sets it to the specified reference, which must either be a compatible probe expression or static reference source. |
| The flow of all other expressions are source. | ||
| - input ports (or subcomponents which are input ports) | ||
| - data field of memory read ports | ||
| - rdata field of memory read-write ports |
There was a problem hiding this comment.
if we use the nomenclature "ports" (as seen from within a module) and "pins" (as seen from outside a module) shouldn't an output pin not be allowed to be referenced with a sink expression?
This PR is my first attempt to remove the concept of "flow" from the FIRRTL spec. Instead, I've replaced it with source/sink expressions and relative alignment.
Note that this is to clarify the underlying concepts which can be used to reason about FIRRTL semantics, but the actual semantics should remain unchanged.
Feedback welcome.
Motivation
"Flow" as a concept to describe FIRRTL connection semantics has (in my opinion) always been a bit lacking in clarity. I would often get confused myself about how it actually worked, even though I first proposed the idea..
As I implemented the
connectableoperators in Chisel, I was able to revisit this concept and made some headway in my own understanding of how to describe connection semantics. As a result, I've been wanted to remove "flow" entirely from the FIRRTL spec and introduce a new concept of "relative alignment" and "source/sink" expressions (or "lref"s, I'm totally open to using more standardized terms).