llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.62k stars 281 forks source link

Generic module/op externalization pass/interface #5638

Open mortbopet opened 1 year ago

mortbopet commented 1 year ago

seq.clock_gate already has it's own externalization pass (--externalize-clock-gate), and in #5630 - which introduces a somewhat complex FIFO lowering - it'd also be natural to have a way to let a user bring their own FIFO module.

This seems like a general issue that probably will reoccur many times over in CIRCT. Hence, it seems worthwhile to figure out if/how such behavior can be shared into a generic op externalization pass. This, to me, smells like we should implement an ExternalizeableInterface to enable such a pass.

CC @darthscsi @teqdruid @fabianschuiki

fabianschuiki commented 1 year ago

Yeah this is a cool idea! We could cook up an op interface where operations could return some key/struct describing their parametrization, plus how to create the extmodule from that parametrization and how the operands map to the extmodule ports. The pass could then collect all keys, group ops by key, create the extmodules, create the instances, and then ask all the ops to adapt to those instances.

Edit: Just saw that that's exactly what you proposed 😃

fabianschuiki commented 1 year ago

LowerFirMems does a similar thing. It looks at some ops, asks each for the extmodule parametrization they'd need, groups ops by those parametrizations so there's only one decl generated, creates that decl, and translates the ops into instances.

teqdruid commented 1 year ago

I'm wondering if a simple dialect attribute would work. It'd be a module name and a list of port names corresponding to the operands and results in order. Having it be a dialect attribute would be more flexible.

mortbopet commented 1 year ago

I'm wondering if a simple dialect attribute would work. It'd be a module name and a list of port names corresponding to the operands and results in order. Having it be a dialect attribute would be more flexible.

Not sure i'd be a fan. This puts a load on something (probably the user) to generate that dialect attribute. However, the logic for generating that dialect attribute is exactly what should be captured in an ExternalizeableInterface. Fair enough, if we have an influx of external dialects that suddenly want to be externalizeable, then it'll be annoying for them to have to inherit this interface. But i don't think that's something we want to optimize for, rather, having it as an interface ensures a much more predictable way of going about this (+ ops should be able to do more complex stuff for their externalization. I see having it as a dialect attribute to be fairly restricting in how ops interface with this).

teqdruid commented 1 year ago

One of the problems with an OpInterface is that it mixes lowering logic with op definition. What about the case where a lowering flow wants to lower an arbitrary op (meaning in a dialect which it has no control over) using an external module? I'm assuming that to be a common case.

What we probably want is not so much a pass as a helper something which is easily executed from a pass. After all, a pass should define the lowering, not the dialect. I'm imagining some subclass of OpConversionPattern which has a reasonable set of defaults (which are overridable) for common cases (e.g. mapping operands/results directly to ports). It should also have a "declare external module" method which is called once for each external mod (again, with a reasonable default implementation). This would make it trivial to support the common cases, while making it easier to support the more complex ones while letting individual passes define the external lowerings.