Enable @chiselName on non-module classes #1209
Conversation
|
Can one of the admins verify this patch? |
|
Does this affect #1046? |
|
@edwardcwang Well, I don't know... it does not break the existing tests but I am not sure they are fully exhaustive ? |
|
ok to test |
|
Any chance to get a review on this one ? @azidar ? |
|
@ducky64 could you take a look at this? Looks very useful |
| @@ -118,12 +118,14 @@ class NamingTransforms(val c: Context) { | |||
| def transformModuleBody(stats: List[c.Tree]): Tree = { | |||
There was a problem hiding this comment.
Comment needs to be revised, the class may not be the naming top level. You might also want to rename associated items like ModuleTransformer and transformModuleBody so that it doesn't imply it only applies to Modules anymore, but handles all classes. Documentation in Namer.scala also needs to be updated.
There was a problem hiding this comment.
Done. I didn't see any comments to be updated in Namer.scala though since the case of Module is described as a special behaviour.
Closing context on a non-module is not an issue.
The only change to Namer.scala I would suggest is to rename namePrefix as closeContext as it sounds a bit clearer to me.
| $contextVar.namePrefix("") | ||
| $globalNamingStack.popReturnContext((), $contextVar) | ||
| if($globalNamingStack.length == 1){ | ||
| $contextVar.namePrefix("") |
There was a problem hiding this comment.
This made sense when Module was top-level, since in all cases the first naming prefix you get would correlate directly with something in the Module. In this case, you could have a non-Module class as the top-level, and the first naming prefix would not necessarily correlate with anything in the Module. Is this behavior we want? Or do we want a check somewhere that the naming top-level is the Module?
There was a problem hiding this comment.
As far as I understand the macro, when top level is not a module what does the transform is harmless and does not reflect in anyway to the emitted names so it is not an issue.
There was a problem hiding this comment.
The use-case is pretty simple : be able to play with oriented-hardware-object programming.
The idea is to have standard classes that are not module but still contains some hardware objects like wire and registers.
I see this non-module hardware classes as tiny reusable blocks on which I implement some methods depending on the context.
Proper naming is required for any debugging related to the contained hardware registers and wires.
It is somehow like an InlineModule but without the wrapping into Module(). Moreover seeing these little reusable blocks as module would lead to great confusion when it comes to hierarchy representation.
There was a problem hiding this comment.
This looks fine.
I'm not completely convinced that this is the right approach to your problem. It seems that you still want a Module, just inlined so the Verilog generation is cleaner. That aside, you can use the companion object pattern to avoid Module(...) wrappers while providing a default IO connection as argument syntax.
This solution seems like a bit of a hack, would it be cleaner just to have some way to declare a Module inline, eg a inline module annotation or mixin?
|
It's not that uncommon Chisel style to have non-Module classes for generating hardware. For example, in this repo: https://github.com/freechipsproject/chisel3/blob/410f03b9122978e43db938d7774b451f2b9111d0/src/main/scala/chisel3/util/Counter.scala#L27 |
|
I can hear your point of view @ducky64, but I agree with @jackkoenig I definitely think that being able to build hardware generator objects that are neither fully-featured modules nor functions is interesting. |
|
@Mergifyio backport 3.2.x |
(cherry picked from commit 9406e2b)
|
Command
Hey, we reacted but our real name is @Mergifyio |
Type of change: other enhancement
Impact: API addition (no impact on existing code)
Development Phase: implementation
Release Notes
Enable
@chiselNameon non-module classesHardware val declared inside the scope of a standard scala class (ie that do not extend any kind of
Module) can now benefit from the@chiselNamemacro and get hw-user-friendly names thanks to the existing fancy macro work of @ducky64 & @jackkoenig =)