[FIRRTL] default layer specialization #7401
Conversation
There was a problem hiding this comment.
The one thing that may be weird about this is that there are now a lot of different ways to do the same thing given that the options include both the ability to enable and disable layers as well as to set the default behavior. Use of mutually exclusive options instead of composable options may be simpler for users: (1) enable all, (2) disable all, and (3) enable specific.
| // CHECK-LABEL: firrtl.circuit "DefaultSpecialization" | ||
| firrtl.circuit "DefaultSpecialization" attributes { | ||
| // This option does not get removed - it is idempotent to leave it. | ||
| // CHECK: default_layer_specialization = #firrtl<layerspecialization enable> | ||
| default_layer_specialization = #firrtl<layerspecialization enable> | ||
| } { | ||
| firrtl.extmodule @DefaultSpecialization() | ||
| } |
There was a problem hiding this comment.
I agree with this. It originally seemed incongruous to remove the attributes for enabling/disabling, but leave this one. However, this one does make sense to leave in.
a3fe29e to
b66df64
Compare
| @@ -35,7 +35,8 @@ def CircuitOp : FIRRTLOp<"circuit", | |||
| StrAttr:$name, | |||
| DefaultValuedAttr<AnnotationArrayAttr, "{}">:$annotations, | |||
| OptionalAttr<SymbolRefArrayAttr>:$enable_layers, | |||
| OptionalAttr<SymbolRefArrayAttr>:$disable_layers | |||
| OptionalAttr<SymbolRefArrayAttr>:$disable_layers, | |||
| OptionalAttr<LayerSpecializationAttr>:$default_layer_specialization | |||
There was a problem hiding this comment.
Minor consideration:
Does it make sense for this to be DefaultValuedAttr defaulting to, "none"? This is (maybe) simpler to store (value, not optional<value>) and means you can just get the behavior directly instead of interpreting no setting as "none" anyway?
Especially since it's never removed. Just a thought.
There was a problem hiding this comment.
I made the attribute only have two cases, enable and disable, and kept it optional on the circuit. I made the command line argument use a different 3 value enum, with None, Enable, Disable. Unfortunately the cl framework doesn't seem to have magical handling for std::optional, and I would have had to use a custom parser to make std::optional<LayerSpecialization> work - and using a new 3 value enum value gives better help descriptions.
This adds a new LayerSpecialization attribute, which will eventually be used to specify the default mode of specialization on a circuit.
This adds a default layer specialization mode attribute to the FIRRTL CircuitOp. This will be used by the layer specialization pass to specialize layers that were not explitly enabled or disabled.
This adds a command line option to specify the default layer specialization mode.
b66df64 to
32ef764
Compare
This modifies the SpecializeLayers pass to understand the default layer specialization mode specified on a circuit.
32ef764 to
b3104c9
Compare
No description provided.