[HW][SV] Transition "HWToSV" to "ProceduralCoreToSV" pass.

[fzi-hielscher]

This PR replaces the "HWToSV" conversion pass with the functionally identical "ProceduralCoreToSV". For the moment, it only converts the hw.triggered operation to sv.always. The motivation for this change is to allow the lowering of the sim.proc.print operations added in #7292 without using the dialect conversion framework, due to its limitations, and without having to mix procedural core operations with procedural SV operations (cc #7314).

On a more general note, I understand the concerns about having procedural regions in the core dialects voiced by @uenoku in the linked PRs. I also think we should keep the synthesizable portions of the design as dataflow-ish as possible. But I don't really see a viable option of pretending there is no control-flow in simulation. Maybe the answer here is to just turn hw.triggered into sim.triggered?

[fabianschuiki]

I like the idea of sim.triggered :thinking:! Doing so would clearly mark this as a construct geared towards simulation.

[fzi-hielscher]

Yeah. The way I lower sim.print at the moment, I need operations from no less than four dialects. Sim, obviously, HW for the TriggeredOp, Seq for converting a !seq.clock value to the trigger and SCF for the condition. I kept wondering whether this is MLIR working as intended or just a massive headache. I seems tempting to consolidate that to something like

sim.triggered posedge %someSeqClock (%someCondition) {
 [...]
  sim.if %arg0 {
    sim.proc.print [...]
  }
}

A motivation for having sim.if would be to disallow results, in contrast to scf.if.

[uenoku]

+1 for sim.triggered! In that case maybe we can just implement ProceduralRegion to sim.triggered regarding dialect dependecy?

[uenoku]

I think we can allow scf.if to have results. We can lowered that to sv.reg + sv.if + sv.bpassign in ProceduralCoreToSV.

[fzi-hielscher]

Thank you for your comments.

+1 for sim.triggered! In that case maybe we can just implement ProceduralRegion to sim.triggered regarding dialect dependecy?

Would you keep the declaration of ProceduralRegion in SV or should we move it to Sim? I think the latter makes more sense if we consider Sim to be one of the core dialects.

I think we can allow scf.if to have results. We can lowered that to sv.reg + sv.if + sv.bpassign in ProceduralCoreToSV.

Good point. I guess that should work.

Was a there specific reason that removes OpConversion pattern and does manual walk? I think we generally prefer OpConversion pattern for the maintainability.

The drawbacks of the Dialect Conversion framework listed in the linked Discourse thread have made me skeptical about it. This may be an overreaction. Of course there is benefit in keeping the code idiomatic. But is it worth it if it is a "bad" idiom and we don't actually need its features? I have also implemented the lowering of the TriggeredOp's body as a single walk and it seemed pretty straightforward to me. But this is not a hill I am going die on. If the consensus is that we prefer the conversion framework (or alternatively the GreedyPatternRewriteDriver) I'll change it.

Can we add `hasParentOp<"hw::HWModuleOp"> to TriggeredOp instead?

I'd say this depends on how we want to deal with ifdef guards. The FIRRTL frontend wraps print operations in sv.ifdef @SYNTHESIS which would then become a problem. On the other hand, if we go for sim.triggered maybe that guard should be inherent to the operation, so we would not have to add it in the frontend? I would actually love that. The guards are a major hassle during lowering. 😅

[uenoku]

Would you keep the declaration of ProceduralRegion in SV or should we move it to Sim? I think the latter makes more sense if we consider Sim to be one of the core dialects.

Yeah moving ProceduralRegion to Sim looks direction to me.

I have also implemented the lowering of the TriggeredOp's body as a single walk and it seemed pretty straightforward to me. But this is not a hill I am going die on

That's fair. In that case I think we don't have to define ProceduralOpRewriter since it's just a wrapper of OpBuilder if it's not used in the rewriter framework.

I'd say this depends on how we want to deal with ifdef guards. The FIRRTL frontend wraps print operations in sv.ifdef @SYNTHESIS which would then become a problem.

Hmm, I see. In that case the current implementation looks reasonable to me.