Support separately elaborating definition and instance in ChiselStage #2512

debs-sifive · 2022-04-29T23:03:48Z

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 state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

new feature/API

TECH DEBT

Need to improve the API to instantiate previously-defined Definitions in the future (right now, we are passing in a Definition into the Top of the second generator call). Tracked in #2514

Need to improve the code that handles module name collisions. Right now I am making a bunch of ImportedDefinitionAnnotations on the imported Definition and all of its submodules; this list of Annotations must get passed around between elaboration stages to ensure no name collisions. Long-term solution may require the support of namespaces in FIRRTL.

API Impact

None.

Backend Code Generation Impact

None.

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

Support for separately elaborating a Definition and Instance.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala

core/src/main/scala/chisel3/internal/Builder.scala

src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala

mwachs5

Looks good to me! Do you think this can be backported to 3.5?

azidar

Please update tests according to our feedback. Also, we should record in the PR description future work to create an API to instantiate a previously-defined Definition, which is not passing the Definition via an argument to the Top of the second elaboration call.

azidar · 2022-05-02T15:50:21Z

src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala

+
+      // If there is a repeat module definition, FIRRTL emission will fail
+      val firrtl = (new ChiselStage).emitFirrtl(
+        gen = new Testbench(dutDef),


Right now, the only way to instantiate a definition from a previous elaboration is to pass it as an argument to the top module in the next elaboration. This means that we must support a better API (e.g. like Instantiate), prior to release.

core/src/main/scala/chisel3/internal/Builder.scala

src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala

debs-sifive · 2022-05-02T18:56:10Z

@mwachs5 @azidar not sure if it needs to be handled in this PR, but what should we do if there are multiple "previously elaborated" definitions with the same name? There doesn't seem to be a way to catch that right now.

Like if I make two AddOneParameterizeds of different widths they both still think they are named AddOneParameterized.

cc @sequencer this is what we were discussing in chisel meeting

sequencer · 2022-05-02T19:13:22Z

Yes. Currently Builder will handle the name confliction and rename to Queue_1 Queue_2. But in the Stage API there is no way to resolve the naming issue.
Maybe a prefix API might be required here?

mwachs5 · 2022-05-02T19:39:11Z

what should we do if there are multiple "previously elaborated" definitions with the same name? There doesn't seem to be a way to catch that right now.

Can we error?

debs-sifive · 2022-05-02T19:59:23Z

I can make Builder error if it sees more than one ImportedDefinitionAnnotation of the same module type...

But that will totally prohibit passing in more than 1 definition of the same module type, which is probably not what we want long term.

debs-sifive · 2022-05-02T20:01:03Z

Another option is to only allow for a single ImportedDefinitionAnnotation for now and say we only support the testbench use case? 🤷‍♀️

mwachs5 · 2022-05-02T20:07:30Z

I can make Builder error if it sees more than one ImportedDefinitionAnnotation of the same module type...

But that will totally prohibit passing in more than 1 definition of the same module type, which is probably not what we want long term.

It's not just the Definition of the same module type, that definition can't instantiate anything (e.g. a Queue) with Module or Instance that are going to be used in a larger context.

So far we handle this with annotations e.g. nested Module prefixing annotation. Maybe this is a good reason to keep pushing on the chisel prefixing API vs relying on annotations for this, but I don't think it really changes this PR.

debs-sifive · 2022-05-02T22:54:58Z

R.e. multiple imported definitions with the same name, from discussion with @azidar, for this PR we should:

pass a list of ImportedDefinitionAnnotations between serial stage calls. This should solve our issues the two definitions are elaborated serially, but not if they are elaborated in parallel.
in Builder, error on multiple ImportedDefinitionAnnotations that are holding Definitions with the same name

mwachs5 · 2022-05-03T00:06:13Z

I chatted with @seldridge a bit here, and we are thinking we are going to make "Namespaces" a more first-class citizen sooner or later. But not sure it's what we want to solve in this PR

debs-sifive · 2022-05-03T17:39:12Z

core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala

+    val existingMod = Builder.components.map {
+      case c: DefModule if c.id == definition.proto          => Some(c)
+      case c: DefBlackBox if c.name == definition.proto.name => Some(c)
+      case _ => None
+    }.flatten


Jack previously pointed out that this is a linear search. Would it be worthwhile to change Builder.components from an ArrayBuffer to a HashMap for better lookup speed?

I don't think its worthwhile unless you can measure the speedup; your lookup function would be complicated because you'd have to find the DefBlackBox case anyways.

core/src/main/scala/chisel3/internal/Builder.scala

mwachs5

looks good to me, slight suggestion for better failing require error messages!

core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala

mwachs5 · 2022-05-03T21:34:24Z

Looks good! Do you want me to label with please merge?

sequencer · 2022-05-03T21:54:30Z

core/src/main/scala/chisel3/internal/Builder.scala

+  if (importedDefinitionNames.distinct.length < importedDefinitionNames.length) {
+    val duplicates = importedDefinitionNames.diff(importedDefinitionNames.distinct).mkString(", ")
+    throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates")
+  }


What if two different definitions have a same sub module(Queue) which has a same name? will it conflict?

Not sure what you mean by "conflict". The top-level module should be able to access the submodules uniquely, since the Definitions are now forced to have unique names.

The submodules also won't conflict in the Builder, since each Definition is built in a separate call to ChiselStage.

you could in theory pass in all the Modules as definitions from the first elaboration, then potential conflicts could be detected/avoided. If yo only pass in the top level definition you'd likely have issues in firrtl/verilog linking later which is still only hypothetical

Yes, I think the question is how we would have to do linking. @sequencer and @mwachs5 make good points, where we won't be able to link easily, without renaming conflict names when combining the Verilog. We could avoid this by adding submodule names to the duplicates.

This is probably the best intermediate solution, because it guarantees no name conflicts.

debs-sifive · 2022-05-03T22:12:17Z

Looks good! Do you want me to label with please merge?

Yes please! @azidar can you please mark the "changes requested" as resolved if your concerns have been addressed?

debs-sifive · 2022-05-05T22:45:36Z

I'm changing this to a draft PR since, after some discussion, I think it makes more sense to solidify what we're doing with namespaces/prefix API and apply that to our multiple-definition-elaboration scheme.

Still working on test cases for submodule name conflicts; will push those soon!

debs-sifive · 2022-05-05T23:09:18Z

src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala

+      dutDef0_rtl should include("module AddOne(")
+      dutDef0_rtl should include("module AddTwoMixedModules(")
+      val dutDef1_rtl = Source.fromFile(s"$testDir/dutDef1/AddTwoMixedModules_1.v").getLines.mkString
+      dutDef1_rtl should include("module AddOne_2(")


@azidar is this expected to skip to AddOne_2 instead of AddOne_1?

@debs-sifive I think what is happening is AddTwoMixedModules made an AddOne (the Definition) and an AddOne_1(the Instance) in the first dutDef0. They may get deduped later but as far as Chisel is concerned you have two names taken up in the namespace by the time you get to dutDef1`

…#2512) (cherry picked from commit a0aa4d1) # Conflicts: # core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala # core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala # core/src/main/scala/chisel3/internal/Builder.scala

… (backport #2512) (#2520) * Support separately elaborating definition and instance in ChiselStage (#2512) (cherry picked from commit a0aa4d1) # Conflicts: # core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala # core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala # core/src/main/scala/chisel3/internal/Builder.scala * fixing imports (#2522) Co-authored-by: Deborah Soung <debs@sifive.com>

debs-sifive added 2 commits April 29, 2022 16:00

first draft

ce1f6d6

whitespace issue

d9503e9