[Seq] Add clock gate conversion to extern module #5364
Conversation
| llvm::cl::opt<std::string, true> ckgModuleName{ | ||
| "ckg-name", llvm::cl::desc("Clock gate module name"), | ||
| llvm::cl::location(clockGateOpts.moduleName), | ||
| llvm::cl::init("EICG_wrapper"), llvm::cl::cat(category)}; | ||
|
|
||
| llvm::cl::opt<std::string, true> ckgInputName{ | ||
| "ckg-input", llvm::cl::desc("Clock gate input port name"), | ||
| llvm::cl::location(clockGateOpts.inputName), llvm::cl::init("in"), | ||
| llvm::cl::cat(category)}; | ||
|
|
||
| llvm::cl::opt<std::string, true> ckgOutputName{ | ||
| "ckg-output", llvm::cl::desc("Clock gate output port name"), | ||
| llvm::cl::location(clockGateOpts.outputName), llvm::cl::init("out"), | ||
| llvm::cl::cat(category)}; | ||
|
|
||
| llvm::cl::opt<std::string, true> ckgEnableName{ | ||
| "ckg-enable", llvm::cl::desc("Clock gate enable port name"), | ||
| llvm::cl::location(clockGateOpts.enableName), llvm::cl::init("en"), | ||
| llvm::cl::cat(category)}; | ||
|
|
||
| llvm::cl::opt<std::string, true> ckgTestEnableName{ | ||
| "ckg-test-enable", | ||
| llvm::cl::desc("Clock gate test enable port name (optional)"), |
There was a problem hiding this comment.
I'm wondering whether there is a better way to handle this. I think practically it looks fine as is, but maybe? in the feature we might want to mix different flavors of clock gates. Another direction would be to introduce dedicated attributes into clock gate ops regarding mapped modules but even with that direction it'll be necessary to add similar options to LowerToHW. So probably the current implementation would be simpler. I guess this is a general problem about intrinsic lowering (how to map intrinsic to external modules, legalize to SV/Comb constructs) so perhaps we might want to revisit in the future.
There was a problem hiding this comment.
Yeah that's a great point; we'll want to revisit this sometime down the road to see if we need more localized lowering specifications. For example, being able to specify a clock gate for some subhierarchy of the design.
My rationale for the current approach is that we currently have no options and no lowering, and this introduces at least a global option and some lowering. Hopefully we can expand this in the future!
There was a problem hiding this comment.
Is there/does there need to be support for the clock divider DFT signal thing too? cc #5219 . Haven't paged this in yet, apologies if that doesn't make sense 👍 .
There was a problem hiding this comment.
Yeah good point, that's definitely something that needs to go into a follow-up PR. Right now this just handles emission of the seq.clock_gate op, which lowers from the firrtl.int.clock_gate intrinsic. The FIRRTL pipeline doesn't support that intrinsic yet in its passes (ExtractTestCode, WireDFT, etc.) -- on my todo list 😏 I totally expect that there will be some extensions to the intrinsic or seq.clock_gate op definitions.
Add the `ExternalizeClockGate` pass which converts all `seq.clock_gate` ops in a design into instances of a `hw.module.extern`. The name and ports of the extern module can be configured through pass options. In case the module has no test enable port, the conversion adds a `comb.or` to fold the enable and test enable operands of the `seq.clock_gate` into a single enable port.
fabianschuiki
force-pushed
the
fschuiki/clock-gate
branch
from
136b129
to
d784879
Compare
Add the
ExternalizeClockGatepass which converts allseq.clock_gateops in a design into instances of ahw.module.extern. The name and ports of the extern module can be configured through pass options. In case the module has no test enable port, the conversion adds acomb.orto fold the enable and test enable operands of theseq.clock_gateinto a single enable port.