Skip to content

Add Incoming/Outgoing as alternatives to IO #3622

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

azidar
Copy link
Contributor

@azidar azidar commented Nov 6, 2023

Motivation

First step in implementing what was discussed in this issue:

#2643

In short, Incoming and Outgoing provide a way to create input/output ports that does not "strip flips" like Input/Output do. The alternative way to do this was to create IO(Flipped(..)) and IO(..), which requires the implicit knowledge that IO creates an output port.

I'm in general curious for feedback on this API, so I'm marking as a draft for now. Thanks!

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
  • API deprecation
  • Backend code generation
  • Performance improvement
  • Bugfix
  • Documentation or website-related
  • Dependency update
  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).
  • Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

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.

@jackkoenig jackkoenig added the Feature New feature, will be included in release notes label Nov 7, 2023
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High level: this makes sense to me and is both better than having an implicit direction and works towards removing the confusion around things often used for port creation (Input/Output) also being useful for interacting with flip.

Bikeshed-level: I'd opt for the terser: In and Out vs. Incoming and Outgoing. This has the coincidence that in/out were the compromise names between favoring terseness and explicitness that were adopted for dialects in CIRCT.

A migration strategy on how to handle existing methods would be good. IO could be made deprecated in preparation for being made private[chisel3]?

Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I'm thinking try to use chisel3.connectable, so maybe this API should leverage Alignment API for connection?

* The granted iodef must be a non-flipped chisel type and not be bound to hardware.
*/
def apply[T <: Data](iodef: => T)(implicit sourceInfo: SourceInfo): T = {
if (SpecifiedDirection.isFlipped(iodef.specifiedDirection))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should require iodef is not coerced yet.

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.

in or before we land this, could we add an Aligned that basically nothing so that people can be writing:

val foo = Flipped(MyThing())
val bar = Aligned(MyThing())

@mwachs5
Copy link
Contributor

mwachs5 commented Nov 9, 2023

Bikeshed-level: I'd opt for the terser: In and Out vs. Incoming and Outgoing. This has the coincidence that in/out were the compromise names between favoring terseness and explicitness that were adopted for dialects in CIRCT.

Adding to the bikeshed i like that:

val foo = Incoming(Bool())
val bar = Outgoing(Bool())

line up 🤷

@sequencer
Copy link
Member

sequencer commented Nov 9, 2023

And another required thing might be providing a Decoupled and Valid equivalent type, and even deprecate the coerced one.

@seldridge
Copy link
Member

What would an explicit Aligned do? Isn't that a no-op?

@sequencer
Copy link
Member

What would an explicit Aligned do? Isn't that a no-op?
How about provide a semantic:

Aligned(Flipped(MyThing()))
Aligned(Aligned(MyThing()))

Both equal to

Aligned(MyThing())

Aligned will ignore the inner Flipped.

@mwachs5
Copy link
Contributor

mwachs5 commented Nov 14, 2023

No @sequencer that is not what I mean. I am agreeing for a noop operator that does nothing, simply for the alignment (no pun intended) of the words.

@darthscsi
Copy link
Contributor

Is it the case that the following are indistinguishable from a connection/flow perspective? (modulo directly inspecting the type)
val a = Incoming(Bundle(flip a : UInt))
val b = Outgoing(Bundle(a : UInt))

@schoeberl
Copy link
Contributor

As @seldridge mentioned, Incoming and Outgoing are unusual names. VHDL uses simply in and out. Verilog uses input and output. Both should be fine.

@mwachs5
Copy link
Contributor

mwachs5 commented Nov 27, 2023

Is it the case that the following are indistinguishable from a connection/flow perspective? (modulo directly inspecting the type)
val a = Incoming(Bundle(flip a : UInt))
val b = Outgoing(Bundle(a : UInt))

Written in actual user Chisel not firrtly-psuedocode:

val a = Incoming(new Bundle{ val a = Flipped(UInt())})
val b = Outgoing(new Bundle{ val a = UInt()})

val a_wire = Wire(new Bundle{ val a = Flipped(UInt())})
val b_wire = Wire(new Bundle{ val a = UInt()})

a :<>= a_wire
b :<>= b_wire

Not the same: https://scastie.scala-lang.org/exwHPt8vSEWpyd7N2SsKmQ

...

connect a_wire.a, a.a @[src/main/scala/main.scala 13:3]
connect b.a, b_wire.a @[src/main/scala/main.scala 14:3]

We can tell the users to write something different, but the above is the common thing people are doing intuitively.

What if the wires have no flips to match:

val a = Incoming(new Bundle{ val a = Flipped(UInt())})
val b = Outgoing(new Bundle{ val a = UInt()})

val a_wire = Wire(new Bundle{ val a = UInt()})
val b_wire = Wire(new Bundle{ val a = UInt()})

a :<>= a_wire
b :<>= b_wire

They get an error: (https://scastie.scala-lang.org/xnj0D741RsqWhoAvSk4hZA)

 src/main/scala/main.scala 13:3: inversely oriented fields Example.a.a: IO[UInt] and Example.a_wire.a: Wire[UInt]
[error] a :<>= a_wire

Copy link

linux-foundation-easycla bot commented Nov 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@mmaloney-sf
Copy link
Contributor

What is the story supposed to be for the enduser?

This kind of feature is technically good. But for this kind of change, what are we going to ask people to do with this new feature? When the designer "needs to add a port", are we advocating the use of Outgoing and Incoming over Input and Output?

@mwachs5
Copy link
Contributor

mwachs5 commented Nov 30, 2023

This kind of feature is technically good. But for this kind of change, what are we going to ask people to do with this new feature? When the designer "needs to add a port", are we advocating the use of Outgoing and Incoming over Input and Output?

IO(...) and IO(Flipped(...)) would be replaced with Outgoing(...) and Incoming(...)

The enduser doesn't write Input or Output today to make a port. they have to write IO(...) and through some complicated relations to whatever is in the ... they get an input or output. In an ideal world we could repurpose the terms Input and Output for this and completely eliminate them from their current usage which is more like "Flip-and-or-Strip" and doesn't really mean Input or Output until you couple it with IO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants