[SV][HW][Sim] Rework (Non)ProceduralOp traits#10195
Conversation
|
|
||
| LogicalResult verifyNotInProceduralRegion(Operation *op) { | ||
| auto *parent = op; | ||
| while ((parent = parent->getParentOp())) { |
There was a problem hiding this comment.
The change in behavior regarding the transitivity of unknown operations makes sense to me 👍
There was a problem hiding this comment.
Thanks! I'm a bit concerned about the potential performance impact added by recursing over the parent ops. But I hope if we consistently apply the ProceduralRegion/NonProceduralRegion trait to the operations that we control, it shouldn't be noticeable.
There was a problem hiding this comment.
Yeah I agree this is negligible for our dialects in tree. It's possible for some users to create deeply nested scf.if etc but I think that's their responsibility not to do that.
6760f4e to
caae11e
Compare
fabianschuiki
left a comment
There was a problem hiding this comment.
LGTM! Thanks for going through all the existing code and fixing things up 💯 👍
Follow up #10180
This moves the
ProcecduralOpandNonProceduralOptraits of the SV dialect to the CIRCT support library, adds them to operations of the HW and Sim dialects, and makes several changes to improve interaction with upstream dialects. The traits only affect operation verification. Compilation results should remain unchanged.Previously, operations carrying the
ProceduralOptrait were only permitted in regions where the immediate parent carried theProceduralRegiontrait. This made it impossible to use these operations in non-SV dialect regions, such asscf.iforfunc.func. To enable this, this PR makes three notable changes to the way these traits work:NonProceduralRegiontrait to mark these regions.(Non)ProceduralOptrait look through parent operations which carry neither theProceduralRegionnor theNonProceduralRegiontrait, so these traits apply transitively.