-
Notifications
You must be signed in to change notification settings - Fork 597
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
Implement auto typeName generation for Records #3504
Conversation
plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala
Outdated
Show resolved
Hide resolved
plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala
Outdated
Show resolved
Hide resolved
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.
Generally looks good but please remove the code duplication in the plugin. Per anonymous Bundle issues, we can deal with that in a follow on PR.
|
||
/** Auto generate a type name for this Bundle using the bundle arguments supplied by the compiler plugin. | ||
*/ | ||
override def typeName: String = autoTypeName(getClass.getSimpleName, _typeNameConParams) |
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.
We should probably think about what the behavior should be for anonymous classes, eg. what if someone writes:
class MyModule(n: Int) extends Module {
val io = IO(new Bundle with HasAutoTypename {
val foo = Input(UInt(n.W))
})
}
I think you will get a name that is something like _packageMyModulee0e14f5_8
which is a pretty terrible name indeed.
I think there's a compelling argument that you should not be able to mix HasAutoTypename into anonymous classes since it's going to be a disaster, and arguably not any inner class (inner classes have their outer object as the first argument, that's where packageMyModuleee0e145f
comes from, that's the .toString
of the instance of MyModule
above).
Also .getSimpleName
throws an exception in some circumstances on Java8 which is still supported by Chisel, see scala/bug#2034 and this is why we have more complicated logic for naming Modules:
def desiredName: String = { |
I'm not going to sandbag this PR of this issue, but more careful handling either needs to be in this PR or come shortly after.
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.
We chatted offline but I think the best path forward is to just find a way to ban inner classes from using HasAutoTypename
in the compiler plugin, then we don't need to worry about the .getSimpleName
issue either because that only affects inner classes.
plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala
Outdated
Show resolved
Hide resolved
class Top(gen: Bundle) extends Module { | ||
val in = IO(Input(gen)) | ||
val out = IO(Output(gen)) | ||
override def desiredName = s"Top_${out.typeName}" | ||
} | ||
|
||
class SimpleBundle(width: Int) extends Bundle with HasAutoTypename { | ||
val foo = UInt(width.W) | ||
} | ||
|
||
class StringlyBundle(width: Int, desc: String) extends Bundle with HasAutoTypename { | ||
val foo = UInt(width.W) | ||
} | ||
|
||
class DataBundle(data: Data) extends Bundle with HasAutoTypename { | ||
val foo = data | ||
} | ||
|
||
class BundleBundle(gen: HasAutoTypename) extends Bundle with HasAutoTypename { | ||
val foo = gen | ||
} |
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.
Please add a test for a Bundle with multiple parameter lists, including implicit eg.
class MultipleParameterLists(x: Int)(y: List[Int])(implicit z: String) extends Bundle with HasAutoTypename { ... }
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.
Approved but please add the requested test and address Megan's comments about the ScalaDoc
Co-authored-by: Megan Wachs <megan@sifive.com>
Adds the
HasAutoTypename
trait, which mixes intoRecord
and generates adef typeName
statement constructed from theRecord
's constructor parameters.Example:
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Squash and merge
Release Notes
Add experimental
HasAutoTypename
traittypeName
forBundle
objects through the compiler plugin.Reviewer Checklist (only modified by reviewer)
3.5.x
,3.6.x
, or5.x
depending on impact, API modification or big change:6.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.