Implement module prefix API by jared-barocsi · Pull Request #2155 · chipsalliance/chisel

jared-barocsi · 2021-10-07T22:29:35Z

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

API Impact

Adds new API

Backend Code Generation Impact

No impact

Desired Merge Strategy

Squash and merge

Release Notes

Implement Module Prefix API

Adds a new object, withModulePrefix(prefix), that prefixes all instantiated modules within its scope with the prefix argument
Can be nested either directly in itself, or within modules created in its scope

Reviewer Checklist (only modified by reviewer)

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

jackkoenig

Overall looks great, nice clean implementation!

jackkoenig · 2021-10-07T23:10:50Z

src/main/scala/chisel3/stage/ChiselStage.scala

+
+  /** Elaborates a Chisel module into a circuit, but return that circuit in module form
+    * @param gen a call-by-name Chisel module
+    */
+  def construct[T <: RawModule](gen: => T): T = {
+    val phase = new ChiselPhase {
+      override val targets = Seq( Dependency[chisel3.stage.phases.Checks],
+                                  Dependency[chisel3.stage.phases.Elaborate] )
+    }
+
+    phase
+      .transform(Seq(ChiselGeneratorAnnotation(() => gen), NoRunFirrtlCompilerAnnotation))
+      .collectFirst {
+        case DesignAnnotation(a) => a.asInstanceOf[T]
+      }
+      .get
+  }


Suggested change

/** Elaborates a Chisel module into a circuit, but return that circuit in module form

* @param gen a call-by-name Chisel module

*/

def construct[T <: RawModule](gen: => T): T = {

val phase = new ChiselPhase {

override val targets = Seq( Dependency[chisel3.stage.phases.Checks],

Dependency[chisel3.stage.phases.Elaborate] )

}

phase

.transform(Seq(ChiselGeneratorAnnotation(() => gen), NoRunFirrtlCompilerAnnotation))

.collectFirst {

case DesignAnnotation(a) => a.asInstanceOf[T]

}

.get

}

This is a very large public API to add just for testing. If we want to add this, we need to be pretty thoughtful about it and with the current architecture of Chisel3, not much value comes from having references to elaborated module objects. Although arguably that should change, that's well beyond the scope of this PR.

jackkoenig · 2021-10-07T23:12:55Z

src/main/scala/chisel3/stage/ChiselStage.scala

 import chisel3.stage.CircuitSerializationAnnotation.FirrtlFileFormat

 import java.io.{StringWriter, PrintWriter}
+import chisel3.Module


Suggested change

import chisel3.Module

Related to comment about `ChiselStage.construct

jackkoenig · 2021-10-08T00:04:40Z

src/test/scala/chiselTests/Module.scala

  }
+
+  property("withModulePrefix should prefix modules within it") {
+    val m = ChiselStage.construct(new Module {


Suggested change

val m = ChiselStage.construct(new Module {

val m = elaborateAndGetModule(new Module {

Related to deletion of ChiselStage.construct, chiselTests.ChiselRunners already defines what you need here: elaborateAndGetModule

jackkoenig · 2021-10-08T00:04:50Z

src/test/scala/chiselTests/Module.scala

+  }
+
+  property("withModulePrefix should support nested module prefixing") {
+    val m = ChiselStage.construct(new Module {


Suggested change

val m = ChiselStage.construct(new Module {

val m = elaborateAndGetModule(new Module {

Related to deletion of ChiselStage.construct, chiselTests.ChiselRunners already defines what you need here: elaborateAndGetModule

jackkoenig · 2021-10-08T00:05:43Z

core/src/main/scala/chisel3/Module.scala

  /** Returns the current Module */
  def currentModule: Option[BaseModule] = Builder.currentModule
+  /** Returns the current nested module prefix */
+  def currentPrefix: String = Builder.getModulePrefix


Suggested change

def currentPrefix: String = Builder.getModulePrefix

def currentModulePrefix: String = Builder.getModulePrefix

Just to avoid confusion with other uses of "prefix" which is usually referring to the intra-Module component prefixing rather than the global namespace Module prefixing.

jackkoenig · 2021-10-08T00:06:25Z

src/test/scala/chiselTests/Module.scala

+
+  property("withModulePrefix should prefix modules within it") {
+    val m = ChiselStage.construct(new Module {
+      val io = IO(new Bundle { })


Suggested change

val io = IO(new Bundle { })

io is no longer needed, please delete from all the added Modules.

jackkoenig · 2021-10-08T00:12:08Z

src/test/scala/chiselTests/Module.scala

+    })
+
+    m.child.nestedChild.name should be ("FooBarModule")
+  }


Because this is a generator, people may want to have generated prefixes, including empty ones. I believe this works but can you add a test showing that an empty prefix (withModulePrefix("")) does not change the resulting modules names?

core/src/main/scala/chisel3/Module.scala

mwachs5

is there any desire to "flush" the prefixing? Or is "nested" the only option?

Eg given:

Module TestHarness
   Module DUT
     Module A
     Module B
   Module VIP

If I say I want everything in TestHarness to be prefixed "TestHarness", e.g. I want TestHarnessVIP, it seems there is no way to "reset" the prefixes for DUT using withPrefix. Is there any other API to "clear out" the prefix stack? So that I don't end up with TestHarnessDUT?

mwachs5 · 2021-10-08T04:31:14Z

src/test/scala/chiselTests/Module.scala

+      }
+    })
+
+    m.child.nestedChild.name should be ("FooBarModule")


can you please add a test which includes a SeqMem and run the ReplSeqMem? What is the prefixing behavior for the seq mems?

And also please add a test for blackbox/inline black box, which should NOT get prefixes applied

Agreed with all of the tests. I'll note that prefixing of SyncReadMems is going to be tricky. I think maybe we should add a test for it but have it as ignore right now because I'm not yet sure how we should handle it.

mwachs5 · 2022-06-21T15:48:32Z

Discussed this again with @jackkoenig . The main limitation of this PR as it is is that we lose the "what is the prefix (ad-hoc namespace)" and "what is the leaf name" distinction. I'd suggest we do one of (in increasing order of complexity / design thought required):

make some convention that names can be . - separated into .fir, and lowered later to concatenate these names (minor firrtl spec expansion). Then make this PR use . to separate the prefixes
Add a first class concept of "prefix" to firrtl spec and pass the prefix in directly
Add a first class concept of "namespace" to firrtl spec and stop using prefix as ad-hoc namespace

Implement module prefixing API

700f18c

jackkoenig requested changes Oct 8, 2021

View reviewed changes

jackkoenig added this to the 3.5.0 milestone Oct 8, 2021

jackkoenig added the Feature New feature, will be included in release notes label Oct 8, 2021

mwachs5 reviewed Oct 8, 2021

View reviewed changes

jardhu added 7 commits October 11, 2021 15:21

Scala doc for withModulePrefix object

b5180f0

Remove ChiselStages.construct

374aafb

Rename 'currentModulePrefix' to 'Module.currentModulePrefix'

bad9f00

Add empty prefix test

6a22f97

Fix compilation errors

86eb496

Add negative blackbox prefixing tests

4a0fb28

Fix blackbox name resolution in namespace

f8f335f

jackkoenig mentioned this pull request Nov 4, 2021

Add SuggestSeqMemName annotation to rename replaced blackboxes chipsalliance/firrtl#2404

Open

14 tasks

mwachs5 mentioned this pull request Jun 21, 2022

Pass optional name in ImportDefinitionAnno used during Separate elaboration of definition and instance #2592

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement module prefix API#2155

Implement module prefix API#2155
jared-barocsi wants to merge 8 commits intochipsalliance:mainfrom
jardhu:module-prefix-api

jared-barocsi commented Oct 7, 2021 •

edited by jackkoenig

Loading

Uh oh!

jackkoenig left a comment

Uh oh!

jackkoenig Oct 7, 2021 •

edited

Loading

Uh oh!

jackkoenig Oct 7, 2021

Uh oh!

jackkoenig Oct 8, 2021

Uh oh!

jackkoenig Oct 8, 2021

Uh oh!

jackkoenig Oct 8, 2021

Uh oh!

jackkoenig Oct 8, 2021

Uh oh!

jackkoenig Oct 8, 2021

Uh oh!

Uh oh!

mwachs5 left a comment •

edited

Loading

Uh oh!

mwachs5 Oct 8, 2021

Uh oh!

jackkoenig Oct 12, 2021

Uh oh!

mwachs5 commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	val m = ChiselStage.construct(new Module {
	val m = elaborateAndGetModule(new Module {

	def currentPrefix: String = Builder.getModulePrefix
	def currentModulePrefix: String = Builder.getModulePrefix

Conversation

jared-barocsi commented Oct 7, 2021 • edited by jackkoenig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

jackkoenig Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwachs5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwachs5 commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jared-barocsi commented Oct 7, 2021 •

edited by jackkoenig

Loading

jackkoenig Oct 7, 2021 •

edited

Loading

mwachs5 left a comment •

edited

Loading