Skip to content
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

Add FixedIORawModule, FixedIOBlackBox #3535

Merged
merged 2 commits into from Sep 20, 2023
Merged

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Sep 18, 2023

Add a private[chisel3] ability to block further IO creation in a
BaseModule. This is intended for advanced, in-tree APIs that need to
disallow the creation of further IO by a user. This API has mutable state
to track if more IO is allowed to be created, a method to get the
value, and a method to set the module to disallow further IO creation.
This one-way setting (h/t @jackkoenig) tries to keep this as locked down
as possible.

Add a new module hierarchy that promotes the type of the IO of that module
to a type parameter. This module hierarchy includes:

  1. PolymorphicIOBaseModule
  2. PolymorphicIORawModule
  3. PolymorphicIOBlackBox

Both (2) and (3) inherit from (1). These modules are all polymorphic in
their type of IO. It follows that no more IO are allowed to be created
other than what is in the type.

Release Notes

Add FixedIORawModule and FixedIOExtModule that have a type parameter describing what their IO is. This is intended for advanced APIs which want to have guarantees about the IO will be. These share a common trait, FixedIOBaseModule which can be used to allow for substitution of one for the other.

@seldridge seldridge added the Feature New feature, will be included in release notes label Sep 18, 2023
@seldridge seldridge force-pushed the dev/seldridge/PolymorphicModule branch 2 times, most recently from b0e5589 to 2827487 Compare September 19, 2023 18:25
@seldridge seldridge changed the base branch from main to dev/seldridge/FlatIO-for-data September 19, 2023 18:26
Base automatically changed from dev/seldridge/FlatIO-for-data to main September 19, 2023 21:58
@seldridge seldridge force-pushed the dev/seldridge/PolymorphicModule branch 2 times, most recently from 960873d to b028444 Compare September 19, 2023 22:32
@seldridge seldridge changed the title Add PolymorphicIOModule Add PolymorphicIOModule, PolymorphicIOBlackBox Sep 19, 2023
Copy link
Contributor

@azidar azidar left a comment

Choose a reason for hiding this comment

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

I'm not in love with the PolymorphicIO name. I feel like most Chisel modules could make the claim that they are polymorphic over their IOs. Isn't it more like a "FixedIO" module?

Anyways, no major issues, I approve.

core/src/main/scala/chisel3/Module.scala Outdated Show resolved Hide resolved
* This module may have no additional IO created other than what is specified
* by its `ioGenerator` abstract member.
*/
sealed trait PolymorphicIOBaseModule[A <: Data] extends BaseModule {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was going to suggest an abstract class here, but I take it back, I think a trait is better.

@seldridge seldridge force-pushed the dev/seldridge/PolymorphicModule branch from b028444 to a42326e Compare September 20, 2023 20:38
@seldridge
Copy link
Member Author

"Fixed" is also a good idea. I think this will much better match what a user thinks about here. "The IO is fixed" vs. "The IO is polymorphic" (???). 👍 Changes to this in the rebase.

* This module may have no additional IO created other than what is specified
* by its `ioGenerator` abstract member.
*/
sealed trait FixedIOBaseModule[A <: Data] extends BaseModule {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on the FixedIO name

/** A generator of IO */
protected def ioGenerator: A

final val io = FlatIO(ioGenerator)
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't the order of trait-mixing-in allow a user to get around this?

trait MySneakyIO {
  val sneakyIO = IO(UInt(32.W))
}

class MyThing extends MySneakyIO with FixedIORawModule 

Copy link
Member Author

Choose a reason for hiding this comment

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

Fortunately, no because FixedIORawModule/FixedIOExtModule are classes and not traits. Those have to be the first thing you mix-in, i.e., they need to follow extends.

A user will see the following compile-time error if they try the above:

[error] /repos/github.com/chipsalliance/chisel/src/test/scala/chiselTests/FixedIOModuleSpec.scala:55:43: class FixedIORawModule needs to be a trait to be mixed in
[error]     class MyThing extends MySneakyIO with FixedIORawModule[Bool](Bool())
[error]                                           ^

If they change the order to be legal Scala, they'll get a runtime error because the trait constructor body runs after IO creation is disallowed:

[info]   java.lang.IllegalArgumentException: requirement failed: This module cannot have user-created IO
[info]   at ... ()
[info]   at chiselTests.FixedIOModuleSpec$MySneakyIO$1.$anonfun$sneakyIO$2(FixedIOModuleSpec.scala:52)
[info]   at chisel3.experimental.prefix$.apply(prefix.scala:50)
[info]   at chiselTests.FixedIOModuleSpec$MySneakyIO$1.$anonfun$sneakyIO$1(FixedIOModuleSpec.scala:52)
[info]   at chisel3.internal.plugin.package$.autoNameRecursively(package.scala:33)
[info]   at chiselTests.FixedIOModuleSpec$MySneakyIO$1.$init$(FixedIOModuleSpec.scala:52)
[info]   at chiselTests.FixedIOModuleSpec$MyThing$1.<init>(FixedIOModuleSpec.scala:55)
[info]   at chiselTests.FixedIOModuleSpec.$anonfun$new$9(FixedIOModuleSpec.scala:57)
[info]   at ... ()
[info]   at ... (Stack trace trimmed to user code only. Rerun with --full-stacktrace to see the full stack trace)

Add a `private[chisel3]` ability to block further IO creation in a
`BaseModule`.  This is intended for advanced, in-tree APIs that need to
disallow the creation of further IO by a user.  This API has mutable state
to track if more IO is allowed to be created, a method to get the
value, and a method to set the module to disallow further IO creation.
This one-way setting (h/t @jackkoenig) tries to keep this as locked down
as possible.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add a new module hierarchy that promotes the type of the IO of that module
to a type parameter.  This module hierarchy includes:

  1. FixedIOBaseModule
  2. FixedIORawModule
  3. FixedIOBlackBox

Both (2) and (3) inherit from (1).  These modules are all "fixed" in the
type of their IO.  It follows that no more IO are allowed to be created
other than what is in the type.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge force-pushed the dev/seldridge/PolymorphicModule branch from a42326e to a79a20e Compare September 20, 2023 21:16
@seldridge seldridge merged commit a79a20e into main Sep 20, 2023
4 checks passed
@seldridge seldridge deleted the dev/seldridge/PolymorphicModule branch September 20, 2023 21:16
@mwachs5
Copy link
Contributor

mwachs5 commented Sep 20, 2023

nit: can you change the name of this PR because i think it shows up in the release notes?

@seldridge seldridge changed the title Add PolymorphicIOModule, PolymorphicIOBlackBox Add FixedIORawModule, FixedIOBlackBox Sep 20, 2023
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.

None yet

3 participants