[Sim] Implement the lowering logic from sim.proc.print to the SV dialect#10172
[Sim] Implement the lowering logic from sim.proc.print to the SV dialect#10172
Conversation
d2a978d to
5358f98
Compare
fzi-hielscher
left a comment
There was a problem hiding this comment.
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:
- Find and convert the print operations
- Run Region DCE on the bodies of the affected
hw.triggeredops to remove the dangling formatting ops. - Run the dialect conversion
62e153a to
ee209ff
Compare
19f96ea to
4e6adb2
Compare
| return success(); | ||
| } | ||
|
|
||
| static Operation *findProceduralRoot(Operation *op) { |
There was a problem hiding this comment.
After using proceduralize-sim, we need to lower hw.triggered to the sv dialect before we can lower sim.proc.print to sv.fwrite; otherwise, the constraints of sv.fwrite cannot be satisfied. This is why I haven't added hw.triggered to the whitelist here.
|
|
||
| // ----- | ||
|
|
||
| hw.module @unsupported_procedural_root(in %clk : i1) { |
There was a problem hiding this comment.
The hw.triggered should be lower to SV dialect first as sv.fwrite is only allowed to appear in the procedure block of sv.
672d519 to
3911e4b
Compare
fabianschuiki
left a comment
There was a problem hiding this comment.
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?
91262c9 to
e314a09
Compare
That sounds reasonable. I must have misunderstood some of the previous reviews, sorry. |
e314a09 to
d0dae92
Compare
Yeah.. I now remember the problem with the SV procedural region trait : #7314 🫤 |
|
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? |
|
Having a common trait would be great 🙏 I encountered this a few times in the past and it was really annoying to avoid. |
82c4417 to
dbf232d
Compare
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. |
dbf232d to
7ac0123
Compare
…ect. AI-assisted-by: OpenAI ChatGPT
7ac0123 to
89caafa
Compare
This PR lowers
sim.proc.printto SystemVerilogsv.fwrite.It only handles the procedural form of print operations. Non-procedural
sim.printis expected to be converted first, for example by--sim-proceduralize.This PR does not add stream support yet.
Split from #10146 .
AI-assisted-by: OpenAI ChatGPT