ExportVerilog emits ports preserving their order. #5738
Conversation
This implements a parser for a new module style. This module style uses ModuleType to store it's ports. This allows maininging port order. The added HWTestModuleOp is temporary to allow development until moving all the modules over.
darthscsi
requested review from
dtzSiFive,
Dinistro,
mortbopet,
lucas-rami,
seldridge,
teqdruid,
rachitnigam,
cgyurgyik,
mikeurbach,
andrewb1999,
fabianschuiki and
maerhart
as code owners
test/Dialect/HW/basic.mlir
Outdated
| @@ -227,3 +227,9 @@ module { | |||
| hw.instance "h2" sym @inst_0 @C() -> () {circt.globalRef = [#hw.globalNameRef<@glbl_D_M1>, #hw.globalNameRef<@glbl_D_M2>, #hw.globalNameRef<@glbl_D_M3>]} | |||
| } | |||
| } | |||
|
|
|||
| module { | |||
| hw.testmodule @NewStyle (input %a : i3, output %b : i3, inout %c : i64 {hw.exportPort = #hw<innerSym@symA>}) { | |||
There was a problem hiding this comment.
Is it worth parsing/printing output ports without the % since they're not actually ssa values?
There was a problem hiding this comment.
There's a couple other bits unsolved here related to ports with invalid SSA names. I've been pushing that a bit down the road.
There was a problem hiding this comment.
Very cool. Love how this unifies the port handling across all those module-like things that all kind of do the same thing, but not quite. The mixing of inputs and outputs is also very neat.
Are you planning to throw out the test module before landing the PR, or does that stay in as a reference for some time?
| emitPortList(module, module.getPorts()); | ||
| emitPortList(module, module.getPortList()); | ||
|
|
||
| assert(state.pendingNewline); | ||
|
|
||
| // Emit the body of the module. | ||
| StmtEmitter(*this).emitStatementBlock(*module.getBodyBlock()); | ||
| startStatement(); | ||
| ps << "endmodule" << PP::newline; | ||
| setPendingNewline(); | ||
|
|
||
| currentModuleOp = nullptr; | ||
| } | ||
|
|
||
| void ModuleEmitter::emitHWTestModule(HWTestModuleOp module) { | ||
| currentModuleOp = module; | ||
|
|
||
| emitComment(module.getCommentAttr()); | ||
| emitSVAttributes(module); | ||
| startStatement(); | ||
| ps << "module " << PPExtString(getVerilogModuleName(module)); | ||
|
|
||
| // If we have any parameters, print them on their own line. | ||
| emitParameters(module, module.getParameters()); | ||
|
|
||
| emitPortList(module, module.getPortList()); |
There was a problem hiding this comment.
With the port list simplified we might even be able to move some of these into a custom parser/printer directive inside the regular assembly format.
| if (key == "input") | ||
| dir = ModulePort::Direction::Input; | ||
| else if (key == "output") | ||
| dir = ModulePort::Direction::Output; |
There was a problem hiding this comment.
Random nit: how about in and out like in the FIRRTL dialect?
It needs to go, but not until HW*ModuleOP is changed, which is the bigger lift. |
darthscsi
changed the title
Using the HWTestModuleOP for testing, ExportVerilog is updated to use the port order in the module when emitting instances and modules. HWModuleOP and company are still input,output ordered until their implementation is migrated to the one used in hwtestmoduleop.
The added HWTestModuleOp is temporary to allow development until moving all the modules over.