circt
circt copied to clipboard
llvm
[ExportVerilog] Make ExportVerilog robust for HWModuleLike, NFC
This is separated from https://github.com/llvm/circt/pull/6977. This changes pre-passes for ExportVerilog to run on HWModuleLike instead of HWModuleOp. https://github.com/llvm/circt/pull/6977 is going to add a support for SV func op and legalization needs to be ran on SV func as well.
I'm not sure this is a good idea. Ops that implement hwmodulelike don't necessarily have semantics which directly map to systemverilog semantics. Have you searched for ops that implement it to ensure that we want them to be directly emitted to verilog?
To be clear, I'm not sure it's a bad idea either.
Thank you for the feedback and that's good point. I thought HWModuleOps feed into ExportVerilog are assumed to be emittable as SV but I agree that it would be nicer to explicitly check that.
I'd like to propose to use Emittable trait https://github.com/llvm/circt/blob/7073e2cfdcd1c76d4aec47be9138663412a570e6/include/circt/Dialect/Emit/EmitOpInterfaces.td#L18-L20 and run PrepareForEmission on HWModuleLike with Emittable trait (or extend Emittable trait to op interface).
Thank you for the feedback and that's good point. I thought HWModuleOps feed into ExportVerilog are assumed to be emittable as SV but I agree that it would be nicer to explicitly check that.
I'd like to propose to use Emittable trait
https://github.com/llvm/circt/blob/7073e2cfdcd1c76d4aec47be9138663412a570e6/include/circt/Dialect/Emit/EmitOpInterfaces.td#L18-L20
and run PrepareForEmission on HWModuleLike with Emittable trait (or extend Emittable trait to op interface).
Does ExportVerilog / PrepareForEmission already depend on (know about) the Emission dialect?
I'm more comfortable with HWModuleLike ops to have to opt-in to Emittable. I'd be even more comfortable if Emittable were an OpInterface with a bool emittedToVerilog() method.
This raises the question: why do we want all HWModuleLike ops to be capable of being emitted to Verilog? Why not lower them into HWModuleOps? Is there a specific use-case?
Or is this intended for preparation only? In that case, I think that any op which implements a future method like Emittable.shouldBePreparedForSVEmission() (or whatever name) should get legalized -- regardless of whether or not it implements HWModuleLike. Given that you probably need things like ports when preparing, though, this can be considered future work so long as it's documented as a restriction.
Does ExportVerilog / PrepareForEmission already depend on (know about) the Emission dialect?
Yes.
This raises the question: why do we want all HWModuleLike ops to be capable of being emitted to Verilog? Why not lower them into HWModuleOps? Is there a specific use-case?
sv.func is the first use case that is not HWModuleop and also needs PrepareForeEmission. HWModuleLike provides sufficient interface for PrepareForEmisson especially regarding port name legalization.
Or is this intended for preparation only?
Yes. PrepareForEmission and probably PrettifyVerilog.
ok. I have no problem with this assuming that you do something with the Emittable trait.
@teqdruid Sorry for the late update. I iterated a bit more and I'm inclined to introduce a new interface HWEmittableModuleLike which inherits HWModuleLike and Emittable to run SV specific legalization.
PrepareForEmission, PrettifyVerilog and HWLegalizeModule should be converted into interface pass (in the current PR only PrepareForEmission is modified).
I thought it might make sense to put this interface to SV but I end up adding the interface to HW due to the dependency around HWModuleLike.
This approach sounds reasonable to me. Had a quick look through the diff and it looks about right to me.