[FIRRTL][LowerLayers] Clean up names of artifacts generated by layers #6733
Conversation
rwy7
commented
Feb 21, 2024
rwy7
commented
- Layer module name: move into helper.
- Layer instance name:
- don't put the parent module name in the instance name,
- lowercase each part of the instance name, not just the first word.
- Layer file name: move into helper.
- Captured port names: don't prefix the port name with an underscore.
rwy7
requested review from
dtzSiFive,
darthscsi and
seldridge
as code owners
|
We need a verifier that bans layernames which are empty or include an underscore. |
Good catch. If this is going to be a problem with the ABI, then we should put it in the spec. |
- Layer module name: move into helper. - Layer instance name: - don't put the parent module name in the instance name, - lowercase each part of the instance name, not just the first word. - Layer file name: move into helper. - Captured port names: don't prefix the port name with an underscore.
| auto operandName = getFieldName(FieldRef(operand, 0), true); | ||
| const auto &[portName, _] = getFieldName(FieldRef(operand, 0), true); |
| auto moduleName = newModule.getModuleName(); | ||
| auto instanceName = | ||
| (Twine((char)tolower(moduleName[0])) + moduleName.drop_front()).str(); | ||
| auto instanceName = instanceNameForLayer(layerBlock.getLayerName()); |
|
|
||
| /// For all layerblocks `@A::@B::@C` in a circuit called Circuit, | ||
| /// the output filename is `layers_Circuit_A_B_C.sv`. | ||
| static SmallString<32> fileNameForLayer(StringRef circuitName, |
There was a problem hiding this comment.
Does this change for FIRRTL 4.0?
abi.md presently reads:
filename = "layers_" , module , "_", root , { "_" , nested } , ".sv" ;
(module not circuit)
Is this going to work for public modules that aren't the main module as-is?
There was a problem hiding this comment.
I think this pass does not do the right thing for public modules. This happens to work now for the main module. The pass should be updated to emit the collateral for all public modules in a follow-on. Good catch, Will.
There was a problem hiding this comment.
Opened an issue: #6748
