-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ExportVerilog][HW] Introduce HWEmittableModuleLike interface and use it for Prepare, NFC #7004
Conversation
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 circt/include/circt/Dialect/Emit/EmitOpInterfaces.td Lines 18 to 20 in 7073e2c
|
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 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 |
Yes.
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.
Yes. PrepareForEmission and probably PrettifyVerilog. |
ok. I have no problem with this assuming that you do something with the |
0400357
to
952d96c
Compare
…eForEmission accordingly
952d96c
to
5b603f5
Compare
@teqdruid Sorry for the late update. I iterated a bit more and I'm inclined to introduce a new interface 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. |
module.walk([&](HWModuleOp op) { modulesToPrepare.push_back(op); }); | ||
SmallVector<HWEmittableModuleLike> modulesToPrepare; | ||
module.walk( | ||
[&](HWEmittableModuleLike op) { modulesToPrepare.push_back(op); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be walk so will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for iterating on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🥳
This approach sounds reasonable to me. Had a quick look through the diff and it looks about right to me. |
This is separated from #6977. This changes pre-passes for ExportVerilog to run on HWEmittableModuleLike instead of HWModuleOp. #6977 is going to add a support for SV func op and legalization needs to be ran on SV func as well.
HWEmittableModuleLike is a new interface that inherits Emittable+HWModuleLike. I considered to use a trait but we cannot use a trait for pass scheduling so an interface is used.