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

diplomacy: change name generation strategy #1137

Closed
wants to merge 3 commits into from
Closed

Conversation

terpstra
Copy link
Contributor

@terpstra terpstra commented Dec 1, 2017

This PR changes how LazyModules find their names.

Consider:
val foo = LazyModule(new MyModule)
This creates a Chisel instance 'foo' of a Chisel module named MyModule.

You can override still override the instance name using .suggestName on the LazyModule.

Disambiguation is punted to Chisel. This means if you create a sequence like:
val foo = Seq.tabulate(4) { LazyModule(new MyModule) }
all four instances will want to be called 'foo' and Chisel will disambiguate them to foo, foo_1, foo_2, foo_3. Since the disambiguation of instances is done in a namespace local to their shared parent, this policy seems perfectly acceptable in a large design. Code changed elsewhere in the design will not affect the instance path name.

However, disambiguation of module names as performed by Chisel is quite problematic. If you have 100 TLFragmenters in your design and you add a new one, chances are that half of the existing modules will get renamed. That means code changed in a completely unrelated part of the design can cause names to change. Unfortunately, LazyModules get the majority of their parameters from diplomacy, which is itself non-local. Thus, depending on these parameters for your name is also not ok. I don't have a good answer yet, and we need one.

@mwachs5
Copy link
Contributor

mwachs5 commented Dec 1, 2017

This sounds reasonable/ the same as how it works for regular modules. I think the disambiguation of the other module names can, as you say, be tackled in a different PR.

@@ -29,11 +28,9 @@ class SystemBus(params: SystemBusParams)(implicit p: Parameters) extends TLBusWr


private val port_fixer = LazyModule(new TLFIFOFixer(TLFIFOFixer.all))
port_fixer.suggestName(s"${busName}_port_TLFIFOFixer")
Copy link
Contributor

@mwachs5 mwachs5 Dec 1, 2017

Choose a reason for hiding this comment

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

is the "busName" distinction no longer necessary due to new hierarchy? Because I'd be annoyed if I end up with port_fixer and port_fixer_1 instead of these named port_fixers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are now created as children of a module called busName already.

Copy link
Member

@hcook hcook Mar 1, 2018

Choose a reason for hiding this comment

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

and are as of #1190 created under further coupler submodules, e.g.: busname.coupler_to_port_named_x.fixer

@terpstra
Copy link
Contributor Author

terpstra commented Dec 1, 2017

If you changed the logic in a module, you will have to recompile it. This Therefore, I think it is ok for a module's name to depend on it's contents. Changing one part of the design won't then change the names of modules in another part. So a first suggestion might be to take a hash of all the wires inside the module and append a few digits to the Module name. To prevent this change from causing modules to get renamed all the way to the top, the hashcode should not include the names of child modules.

This is IMO better than using the constructor arguments of a Module, because those arguments capture very little of the things which might affect the generated circuit.

@terpstra
Copy link
Contributor Author

terpstra commented Dec 1, 2017

This hash code renaming could also happen later, in firrtl.

We might also want to let a Module decide on the name conflict resolution scheme. Options like:
1- Fail to compile if there are two instances
2- Do the brittle x, x_1, x_2 policy
3- Always append a hash code
4- Only append a hash code once there are more than one of the module
5- Hash your pathName

@mwachs5
Copy link
Contributor

mwachs5 commented Dec 1, 2017

And of course modules should always be able to specify their own Module Name (presumably based on parameters, as is done for the SynchronizerShiftReg. But those aren't lazy anyway so I assume don't really apply to this discussion?)

@@ -136,14 +120,15 @@ object LazyModule
protected[diplomacy] var scope: Option[LazyModule] = None
private var index = 0

def apply[T <: LazyModule](bc: T)(implicit sourceInfo: SourceInfo): T = {
def apply[T <: LazyModule](bc: T)(implicit valName: ValName, sourceInfo: SourceInfo): T = {
Copy link
Member

Choose a reason for hiding this comment

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

Consider introducing this API change now to keep things aligned while we await a solution for Chisel module disambiguation

Copy link
Member

Choose a reason for hiding this comment

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

@hcook
Copy link
Member

hcook commented Mar 1, 2018

Closing as it was subsumed by #1190, ongoing issue tracked in #1259

@hcook hcook closed this Mar 1, 2018
@terpstra terpstra deleted the name-changes branch October 13, 2018 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants