Add `withModulePrefix` #4487

mmaloney-sf · 2024-10-25T23:22:04Z

Introduces a new API withModulePrefix.

Example:

class Submod extends RawModule {
  // ...
}

class Top extends RawModule {
  val noprefix_inst = Module(new Submod)

  withModulePrefix("Foo") {
    val foo_inst = Module(new Submod)
  }

  withModulePrefix("Bar") {
    val bar_inst = Module(new Submod)
  }
}

This will result in the definition of the following modules:

  module Submod :
    skip

  module Foo_Submod :
    skip

  module Bar_Submod : 
    skip

  public module Top :

    inst noprefix_inst of Submod
    inst foo_inst of Foo_Submod
    inst bar_inst of Bar_Submod

Only module definitions are prefixed. Other circuit components, such as wires and registers, are unaffected.

Module prefixes nest, where Foo_Bar_Submod indicating that a prefix Foo exists in the grantparent or higher of the instance of Submod.

When using this API withInstantiate, two instantiations under two distinct module prefix blocks will result in two "copies" of the definition, one for each prefix. Conversely, two instantiations under the same module prefix block will share the same prefixed definition.

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)

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.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.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.

…efix is non-empty.

jackkoenig

Generally looks good, 1 question about _.

Also we need to define and test the behavior for Definition and Instance. Instance should have no interaction with the prefix but should Definition? My initial reaction is "no" since Definition is essentially a separate Chisel elaboration, but I'm unsure. Any thoughts @mwachs5, @azidar, @seldridge?

We also probably need a way to remove the prefix, e.g. withNoModulePrefix or withOverrideModulePrefix("") or something.

core/src/main/scala/chisel3/ModuleImpl.scala

src/test/scala/chiselTests/PrefixSpec.scala

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

src/test/scala/chiselTests/PrefixSpec.scala

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

mwachs5 · 2024-10-28T20:16:34Z

Well, for definitions I think you do want the module prefix baked into it somehow. Is it a Val def = withModulePrefix (Definition(new Foo)) or perhaps it's better something one adds as an argument the Definition/Module/Instantiate APIs directly?

mwachs5 · 2024-10-28T20:18:28Z

Actually now that I wrote the above comment I really think it's a more natural thing to do than withModulePrefix(...) even for Module. For the latter why would we be doing it for more than one child ever, isn't it always started at one module or definition instantiation?

jackkoenig · 2024-10-28T22:17:29Z

Actually now that I wrote the above comment I really think it's a more natural thing to do than withModulePrefix(...) even for Module. For the latter why would we be doing it for more than one child ever, isn't it always started at one module or definition instantiation?

I agree that we should instead make it an argument to Module, Definition, and Instantiate, but I just want to note that it's less about prefixing more than one child at a time, and more about adding 1 new API vs. modifying n (where n appears to be 3, but any other added APIs will also potentially need to add prefix).

So let's proceed with adding prefix as an optional (well optional via overloading) argument to Module, Definition, and Instantiate, but I want to call out some potential future work.

How do we deal with instantiating modules from different prefix "namespaces"? For example, say you have a library of Queues and you want to instantiate from the library rather than forcing the current prefix. We could add another overloaded form of Module, Definition, and Instantiate to "set" the prefix rather than push to it, or we could add something similar to withModulePrefix but just for that use case.
How do we deal with prefix for the top module? Should there be a way from within a Module class to set the prefix for yourself (and all children) or should it just be done on the command-line?

mwachs5 · 2024-10-28T22:35:13Z

So let's proceed with adding prefix as an optional (well optional via overloading) argument to Module, Definition, and Instantiate, but I want to call out some potential future work.

How do we deal with instantiating modules from different prefix "namespaces"? For example, say you have a library of Queues and you want to instantiate from the library rather than forcing the current prefix. We could add another overloaded form of Module, Definition, and Instantiate to "set" the prefix rather than push to it, or we could add something similar to withModulePrefix but just for that use case.

Hm, very good point. I guess it's almost like you are forced to opt-in to making your component a valid 'top level' component that gets to control the prefixing.

How do we deal with prefix for the top module? Should there be a way from within a Module class to set the prefix for yourself (and all children) or should it just be done on the command-line?

This makes me wonder, what is the behavior of doing something like:

val q = Module(withModulePrefix(new Queue))

Do you get an error? Is it the same as withModulePrefix(Module(new Queue))? Probably if not, you should get an error... and if so, isn't that the answer to the question 2?

core/src/main/scala/chisel3/ModuleImpl.scala

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

src/test/scala/chiselTests/PrefixSpec.scala

jackkoenig · 2024-10-28T23:19:12Z

So let's proceed with adding prefix as an optional (well optional via overloading) argument to Module, Definition, and Instantiate, but I want to call out some potential future work.

How do we deal with instantiating modules from different prefix "namespaces"? For example, say you have a library of Queues and you want to instantiate from the library rather than forcing the current prefix. We could add another overloaded form of Module, Definition, and Instantiate to "set" the prefix rather than push to it, or we could add something similar to withModulePrefix but just for that use case.

Hm, very good point. I guess it's almost like you are forced to opt-in to making your component a valid 'top level' component that gets to control the prefixing.

Right, how should we do this? Maybe the answer depends on how we want to do linking.

How do we deal with prefix for the top module? Should there be a way from within a Module class to set the prefix for yourself (and all children) or should it just be done on the command-line?

This makes me wonder, what is the behavior of doing something like:
val q = Module(withModulePrefix(new Queue))
Do you get an error? Is it the same as withModulePrefix(Module(new Queue))? Probably if not, you should get an error... and if so, isn't that the answer to the question 2?

I don't think it's the answer to question (2) though since we're moving to making the prefix an argument to Module, Definition, and Instance unless you're opinion on this has changed lol.

mwachs5 · 2024-10-29T22:01:03Z

don't think it's the answer to question (2) though since we're moving to making the prefix an argument to Module, Definition, and Instance unless you're opinion on this has changed lol.

Yes it has changed because as you pointed out it will not be so uncommon to call

withModulePrefix { HelperObjectThatActuallyCallsModule }

so if we need withModulePrefix anyway not sure we should expose it on the others

core/src/main/scala/chisel3/ModuleImpl.scala

docs/src/explanations/moduleprefix.md

core/src/main/scala/chisel3/ModuleImpl.scala

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

docs/src/explanations/moduleprefix.md

src/test/scala/chiselTests/PrefixSpec.scala

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

src/test/scala/chiselTests/PrefixSpec.scala

…without prefixing it

Co-authored-by: Megan Wachs <megan@sifive.com>

src/test/scala/chiselTests/PrefixSpec.scala

docs/src/explanations/moduleprefix.md

src/test/scala/chiselTests/PrefixSpec.scala

Co-authored-by: Megan Wachs <megan@sifive.com>

src/test/scala/chiselTests/PrefixSpec.scala

mwachs5

approving with minor comments! Thanks for adding this great feature

src/test/scala/chiselTests/PrefixSpec.scala

jackkoenig

LGTM, consider fixing nit

…Prefix.

mmaloney-sf added 5 commits October 16, 2024 12:49

Add withModulePrefix.

7de13e6

Add another test.

4c0b95e

Add ModulePrefixAnnotation on modules instantiated when the module pr…

9bf3474

…efix is non-empty.

Use prefix as a key for Instantiate API, add tests.

8b88c4b

Typo: defination -> definition

242b3fd

seldridge added the Feature New feature, will be included in release notes label Oct 26, 2024

Lint.

f5b4a03

jackkoenig reviewed Oct 28, 2024

View reviewed changes

mmaloney-sf and others added 8 commits October 28, 2024 12:10

Update core/src/main/scala/chisel3/ModuleImpl.scala

9d00afa

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

Update src/test/scala/chiselTests/PrefixSpec.scala

95e2003

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

Update src/test/scala/chiselTests/PrefixSpec.scala

8916362

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

Update src/test/scala/chiselTests/PrefixSpec.scala

1b07e16

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

Remove printlns in tests.

8e1ffb0

Remove unnecessary .toList.

820ffbf

Move seperator (sep) into a variable stored in ChiselContext.

2f46bc3

Move instantiable module into test.

f4c4752

Lint fix.

69c6ec5

mwachs5 reviewed Oct 28, 2024

View reviewed changes

core/src/main/scala/chisel3/ModuleImpl.scala Show resolved Hide resolved

mwachs5 reviewed Oct 28, 2024

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Show resolved Hide resolved

mwachs5 reviewed Oct 28, 2024

View reviewed changes

src/test/scala/chiselTests/PrefixSpec.scala Show resolved Hide resolved

mmaloney-sf added 2 commits October 30, 2024 13:50

Add some docs.

e9f36db

Move instantiable class to an object.

ad99036

seldridge reviewed Oct 31, 2024

View reviewed changes

core/src/main/scala/chisel3/ModuleImpl.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Oct 31, 2024

View reviewed changes

docs/src/explanations/moduleprefix.md Outdated Show resolved Hide resolved

jackkoenig reviewed Nov 8, 2024

View reviewed changes

mmaloney-sf and others added 7 commits November 8, 2024 11:50

Scaladoc for withModulePrefix.

6f843e1

Rename sep

b12231b

BlackBox to md docs.

bdc852a

allow definitions to be instantiated within a withModulePrefix block …

3b66e8f

…without prefixing it

Update src/test/scala/chiselTests/PrefixSpec.scala

2d56620

Co-authored-by: Megan Wachs <megan@sifive.com>

Lint.

4bf53b6

Add missing test for memories and remove commented out code.

a7733d3

jackkoenig reviewed Nov 8, 2024

View reviewed changes

src/test/scala/chiselTests/PrefixSpec.scala Show resolved Hide resolved

mmaloney-sf added 2 commits November 8, 2024 13:08

Reset in scala doc.

19267fc

Add missing test.

f4024e0

jackkoenig marked this pull request as ready for review November 8, 2024 20:35

mmaloney-sf added 2 commits November 8, 2024 14:33

Add currentModulePrefix to Module class.

91a19b2

Rename currentModulePrefix to modulePrefix.

44958bf

mwachs5 reviewed Nov 8, 2024

View reviewed changes

docs/src/explanations/moduleprefix.md Show resolved Hide resolved

Add module prefixing.

2f56177

mwachs5 reviewed Nov 8, 2024

View reviewed changes

docs/src/explanations/moduleprefix.md Show resolved Hide resolved

mwachs5 reviewed Nov 8, 2024

View reviewed changes

src/test/scala/chiselTests/PrefixSpec.scala Outdated Show resolved Hide resolved

Update src/test/scala/chiselTests/PrefixSpec.scala

667da38

Co-authored-by: Megan Wachs <megan@sifive.com>

mwachs5 reviewed Nov 8, 2024

View reviewed changes

src/test/scala/chiselTests/PrefixSpec.scala Show resolved Hide resolved

mwachs5 approved these changes Nov 8, 2024

View reviewed changes

jackkoenig reviewed Nov 8, 2024

View reviewed changes

src/test/scala/chiselTests/PrefixSpec.scala Show resolved Hide resolved

jackkoenig approved these changes Nov 8, 2024

View reviewed changes

mmaloney-sf added 2 commits November 8, 2024 16:22

Add clarification about how to explicitly name ExtModules with module…

214c6ec

…Prefix.

Merge branch 'main' into mmaloney/with-module-prefix

f734587

jackkoenig enabled auto-merge (squash) November 8, 2024 23:51

Imports in docs.

6e1402b

auto-merge was automatically disabled November 8, 2024 23:55
Head branch was pushed to by a user without write access

jackkoenig merged commit 0e19171 into chipsalliance:main Nov 9, 2024
15 checks passed

jackkoenig added this to the 7.0 milestone Nov 9, 2024

Add withModulePrefix #4487

Add withModulePrefix #4487

Uh oh!

Conversation

mmaloney-sf commented Oct 25, 2024

Contributor Checklist

Type of Improvement

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwachs5 commented Oct 28, 2024

Uh oh!

mwachs5 commented Oct 28, 2024

Uh oh!

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

Uh oh!

Uh oh!

mwachs5 commented Oct 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackkoenig commented Oct 28, 2024

Uh oh!

mwachs5 commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwachs5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Add `withModulePrefix` #4487

Add `withModulePrefix` #4487

jackkoenig commented Oct 28, 2024 •

edited

Loading

mwachs5 commented Oct 29, 2024 •

edited

Loading