-
Notifications
You must be signed in to change notification settings - Fork 594
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
Instance/Definition (formerly Template) #2045
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts.
class AddZeroWithCompanionObject extends MultiIOModule { | ||
@public val in = IO(Input(UInt(32.W))) | ||
@public val out = IO(Output(UInt(32.W))) | ||
@public val clock = clock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily like the @public
annotation. Wouldn't normally only the I/Os be public? Is there a way to automatically expose the I/Os without requiring a new annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the explicit annotation, but I don't think @public
is the right word (it's too generic). instancePublic
could be one suggestion, or annotatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are saying by default IOs should be public? That may require some cleverness to get working by default, but I can work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we distinguish between I/Os which might be optimized away and those that might not? Is there any implication if you mark an I/O @to-be-renamed-public
on whether it can be optimized away? that may be an argument for NOT automatically making all I/Os @to-be-renamed-public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimizing away I/Os is a firrtl issue, not really a Chisel issue imho. This means that the firrtl compiler will be able to figure out which I/Os it can get rid of based on how the module is used, Chisel users should not have to worry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimizing away I/Os is a firrtl issue, not really a Chisel issue imho
I think this is only true if FIRRTL should never optimize away I/Os but given it's an intentional feature, I think you need a Chisel-level user API to say either when you should, or shouldn't, optimize ports away. Making it such that ports defined as part of the "public API" seems like it could be convenient/intuitive but perhaps I'm wrong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is only true if FIRRTL should never optimize away I/Os but given it's an intentional feature, I think you need a Chisel-level user API to say either when you should, or shouldn't, optimize ports away. Making it such that ports defined as part of the "public API" seems like it could be convenient/intuitive but perhaps I'm wrong here.
I guess my stance on this had always been that only toplevel I/Os need to be preserved.
However, I think the discussion of whether there should be a way to annotate I/Os as public API should be independent from the Instance API discussion. I just wanted to point out that during Chisel elaboration nothing is optimized away, so there is no technical reason as far as I can see for Chisel users to be concerned about optimized I/Os in the context of the new Instance API.
@@ -239,12 +275,11 @@ package internal { | |||
rec(start) | |||
} | |||
|
|||
private[chisel3] def cloneIORecord(proto: BaseModule)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): ClonePorts = { | |||
private[chisel3] def createIORecord[T <: BaseModule](cloneParent: ModuleClone[T])(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): ClonePorts = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this function we check if proto isInstanceOf[Module], changed from MultiIOModule. Note that in the backport, we have to check MultiIOModule, not Module. I saw this as a bug, so just noting it here so we remember.
2846267
to
894d06b
Compare
Squashed to make rebasing easier and rebased on master (which now has |
TLDR I've removed the implicit conversions to Instance[T], replacing them with Instance.wrap (in addition to the fact that In writing tests I ran into issues where the implicit conversion, in particular val inst: Instance[MyModule] = Module(new MyModule) This will give an error of the form:
This message is almost worthless, and it turns out the problem is that the implicit conversion is inserted inside So the implicit conversion work great sometimes, but there are common use cases where they fall on their head, same problem when trying to mix Modules and Instances (which you might do with BlackBoxes) val template = Definition(new MyModule)
val insts: List[Instance[MyModule]] = List(Module(new MyModule), Instance(template))
// inferred type arguments [chisel3.experimental.hierarchy.Instance[MyModule]] do not conform to macro method apply's type parameter bounds [T <: chisel3.experimental.BaseModule]
// val insts: List[Instance[MyModule]] = List(Module(new MyModule), Instance(template)) (And note that if I didn't have the explicit type parameter, Scala would infer that as a List[Any]) So to avoid these worthless error messages, I've removed the implicit conversion and added I called the method val proto = Definition(new MyModule)
val i0 = Instance(proto)
val i1 = Instance(proto)
// i0 and i1 are different instances
val inst = Module(new MyModule)
val i2 = Instance.wrap(inst)
val i3 = Instance.wrap(inst)
// i2 and i3 are the same instance This change nixes @ekiwi's suggestion that we drop |
b08d68b
to
02a0d48
Compare
How is Reset type inference supposed to work: if I am deep within some module of abstract reset type, if I build my definition there, i would want to indicate the reset type somehow. What is the right way to do that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review
core/src/main/scala/chisel3/experimental/hierarchy/DefinitionLookupable.scala
Outdated
Show resolved
Hide resolved
On the record: this is the biggest single needed improvement to Chisel in it's ~10 year existence from a language perspective. I'm very excited to see this converging. |
EDIT I tried to mark these as Because I don't want to merge commented tests, here are @azidar's Aspect tests that were commented out and are future work: describe("8: Aspect APIs work properly"){
it("8.0: Work annotating via an aspect") {
val aspect = aop.injecting.InjectingAspect(
{adder: AddTwo => Seq(adder)},
{adder: AddTwo =>
mark(adder.i0.innerWire, "rel")
}
)
val mt = "~AddTwo|AddTwo".mt
val rt = "~AddTwo|AddTwo/i0:AddOne>innerWire".rt
check(new AddTwo(), aspect, {
case aop.injecting.InjectStatement(mt, _, _, Seq(MarkAnnotation(rt, "rel"))) => true
})
}
it("8.1: Work injecting via an aspect into a definition") {
val aspect = aop.injecting.InjectingAspect(
{adder: AddTwo => Seq(adder.definition.module)},
{add1: AddOne =>
printf("In Add1")
}
)
import _root_.firrtl.ir._
import _root_.firrtl.Mappers._
val mt = "~AddTwo|AddOne".mt
check(new AddTwo(), aspect, {
case aop.injecting.InjectStatement(mt, b: _root_.firrtl.ir.Block, Nil, Nil) =>
println(b.stmts)
def onStmt(s: Statement): Unit = {
s.foreachStmt(onStmt)
}
b.stmts.collectFirst {
case p: Print =>
case p: Print =>
}.getOrElse(false)
})
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome.
I reviewed the documentation and skimmed over the tests (these are extremely exhaustive). I did find the tests hard to follow due to the liberal use of the overloaded check
method and wonder if these could be refactored to make each test more self-contained. I'm not sure that's worth fixing, though. (And it's something that can certainly happen once this lands if we want to do that.)
src/test/scala/chiselTests/experimental/hierarchy/DefinitionSpec.scala
Outdated
Show resolved
Hide resolved
|
||
Thus, it is important that when converting normal modules to use this package, you are careful about what you mark as `IsLookupable`. | ||
|
||
In the following example, we added the trait `IsLookupable` to allow the member to be marked `@public`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example could benefit from showing what you can do with this. E.g., accessing an instance of MyModule
's public x
.
This introduces a new experimental API for module instantiation that disentagles elaborating the definition (or implementation) from instantiation of a given module. This solves Chisel's longstanding reliance on "Deduplication" for generating Verilog with multiple instances of the same module. The new API resides in package chisel3.experimental.hierarchy. Please see the hierarchy ScalaDoc, documentation, and tests for examples of use. Co-authored-by: Jack Koenig <koenig@sifive.com> Co-authored-by: Megan Wachs <megan@sifive.com> Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
7cc7b80
to
43898b1
Compare
This commit breaks dsptool compiling in https://github.com/sequencer/playground/runs/3519024613?check_suite_focus=true#step:9:3638 |
This commit applys Definition and Instance for some modules. Refer to chipsalliance/chisel#2045.
This commit applys Definition and Instance for some modules. Refer to chipsalliance/chisel#2045.
Starting to work on an official PR. More edits coming! For now, this is a DRAFT PR.
Documentation
To Decide!
@public
. Alternatives:@visible
,@open
,@accessable
,@distinct
,@tangible
,@gettable
,@viewable
Definition
toDef
, orInstance
toInst
?@public
work ondef
? No, because if we will replay, we won't have access to defsCan we expose Definition's underlying module? e.g. for Aspects?For 'replaying' ability, this would be a bad APIInstance
vs just usingModule
-- UsingInstance
because of need forInstance.wrap
To Document!
@instantiable
and@public
To Test!
.toTarget
inside ofDefinition
Check what happens if you do multipleMade it harder to use the apply API, so this is no longer relevant..
in (class)Instance.apply
-- this is probably buggy and needs to be banned by a macroTo Develop!
@public val x = bundle.subfield
bugDefinition
(instead of defaulting toBool
for top-level modules)@instantiable
put extension methods in companion objectIsInstantiable
optional when you have@instantiable
as wellMore conversions (for containers like Seqs/Option)Replaced by explicitInstance.wrap
To Do Later!
Decoupled[T]
)@instantiable
optional@public
optional on ports@public
work on vals in the class constructor@instantiable
implicit class in the type's companion object for better implicit resolutionContributor Checklist
docs/src
?Type of Improvement
API Impact
Only adds APIs. However, note that as of this PR, it is incompatible with some Aspect APIs. Future work will introduce new Aspect APIs which are compatible.
The one exception is we change the
Builder.build
function to acceptBaseModule
, not justRawModule
. However, this change is not exposed to the user.Backend Code Generation Impact
No affect.
Desired Merge Strategy
Or.. I'll rewrite the history? Let me know if you want separate commits.
Release Notes
Prior to this release, Chisel users relied on deduplication in FIRRTL to create multiple instances of the same module. This release's
chisel3.experimental.hierarchy
package introduces the following new APIs to enable multiply-instantiated modules directly in Chisel.Definition(...)
enables elaborating a module, but does not actually instantiate that module. Instead, it returns aDefinition
class which represents that module's definition.Instance(...)
takes aDefinition
and instantiates it, returning anInstance
object.Modules (classes or traits) which will be used with the
Definition
/Instance
api should be marked with the@instantiable
annotation at the class/trait definition.To make a Module's members variables accessible from an
Instance
object, they must be annotated with the@public
annotation. Note that this is only accessible from a Scala sense -- this is not in and of itself a mechanism for cross-module references.In the following example, use
Definition
,Instance
,@instantiable
and@public
to create multiple instances of one specific parameterization of a module,AddOne
.Reviewer Checklist (only modified by reviewer)
Please Merge
?