[Sim] Implement the lowering logic from sim.proc.print to the SV dialect

[nanjo712]

This PR lowers sim.proc.print to SystemVerilog sv.fwrite.

It only handles the procedural form of print operations. Non-procedural sim.print is expected to be converted first, for example by --sim-proceduralize.

Split from #10146 .

AI-assisted-by: OpenAI ChatGPT

[fzi-hielscher]

It is generally not recommended to traverse def-use-chains within the dialect conversion framework, so an OpConversionPattern is not ideal for lowering the print operations. We should perform the lowering of sim.proc.print before doing the dialect conversion. I would suggest to:

1. Find and convert the print operations
2. Run Region DCE on the bodies of the affected hw.triggered ops to remove the dangling formatting ops.
3. Run the dialect conversion

[fabianschuiki]

Really amazing effort @nanjo712 🥳, thanks for pushing this forward! One thing I noticed: you had this neatly tucked into the SimToSV conversion earlier, which feels like the right spot to convert a sim.proc.print op to sv.fwrite. Would it make sense to go back to that? That would eventually allow us to have SimTransforms not link against the SV dialect anymore -- all the patterns that touch both Sim and SV would live in the SimToSV conversion. WDYT?

[nanjo712]

Really amazing effort @nanjo712 🥳, thanks for pushing this forward! One thing I noticed: you had this neatly tucked into the SimToSV conversion earlier, which feels like the right spot to convert a sim.proc.print op to sv.fwrite. Would it make sense to go back to that? That would eventually allow us to have SimTransforms not link against the SV dialect anymore -- all the patterns that touch both Sim and SV would live in the SimToSV conversion. WDYT?

That sounds reasonable. I must have misunderstood some of the previous reviews, sorry.

[fzi-hielscher]

The hw.triggered should be lower to SV dialect first as sv.fwrite is only allowed to appear in the procedure block of sv.

Yeah.. I now remember the problem with the SV procedural region trait : #7314 🫤
If you do the ProcPrint to FWrite conversion as part of the same pass but before applyPartialConversion does this still cause a verification error? Since I've posted that PR, LLHD has also gained its own procedural region trait. Do you think we could unify the LLHD and SV flavor in the HW dialect and add it to hw.triggered, @fabianschuiki?

[fabianschuiki]

It would be great to have a unified trait for that! I wonder whether this could live somewhere in CIRCTSupport or so, since it seems to be mostly orthogonal to the actual dialects. WDYT @fzi-hielscher?

[uenoku]

Having a common trait would be great 🙏 I encountered this a few times in the past and it was really annoying to avoid.

[nanjo712]

The hw.triggered should be lower to SV dialect first as sv.fwrite is only allowed to appear in the procedure block of sv.

Yeah.. I now remember the problem with the SV procedural region trait : #7314 🫤 If you do the ProcPrint to FWrite conversion as part of the same pass but before applyPartialConversion does this still cause a verification error? Since I've posted that PR, LLHD has also gained its own procedural region trait. Do you think we could unify the LLHD and SV flavor in the HW dialect and add it to hw.triggered, @fabianschuiki?

Thanks for following up on this with #10180 and #10195.

Given that, I plan to wait for #10195 to land before continuing this PR. Once that happens, I'll update the checking logic here to follow that design instead of preserving the current ad hoc restrictions.

[uenoku]

Could you split get_file lowering? I think it's better to get plain format string done first.

[nanjo712]

Could you split get_file lowering? I think it's better to get plain format string done first.

This sounds reasonable. I have reduced the current PR to only handle the range without specified output streams. We can focus on this issue first.

[fzi-hielscher]

Thanks for pursuing this, @nanjo712 👍

[fzi-hielscher]

Thanks. The conversion LGTM. Could you change the sv.* ops in the test to HW and SCF ops, so they align with how the pass will be used in practice?

[nanjo712]

Could you change the sv.* ops in the test to HW and SCF ops, so they align with how the pass will be used in practice?

That's resonable. Done.

[fzi-hielscher]

Cool. 🥳 Thanks again for putting in the work.
Just one small thing and a request for the future: If you are making incremental changes due to review comments, please add commits to the PR instead of force-pushing the single commit. The commits get squashed before merging, and it helps me (and others) a lot to track which parts have changed and need re-review.

[uenoku]

Thanks!

[nanjo712]

Thank you for your detailed review!

If you are making incremental changes due to review comments, please add commits to the PR instead of force-pushing the single commit. The commits get squashed before merging, and it helps me (and others) a lot to track which parts have changed and need re-review.

I understand. I wasn't very familiar with the squash mechanism before, sorry for any inconvenience it may have caused.