Skip to content
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

[Sim] Add printing operations and transformation from non-procedural to procedural flavor #7292

Merged
merged 22 commits into from
Jul 17, 2024

Conversation

fzi-hielscher
Copy link
Contributor

@fzi-hielscher fzi-hielscher commented Jul 9, 2024

Follow-up PR to #7208. Adds operations for formatted printing during simulation.

The print operation comes in two flavors: The non-procedural sim.print operation takes as arguments a single (concatenated) !sim.fstring value, a boolean condition value to enable the print, and a clock that triggers the print on its rising edge.
The procedural sim.proc.print takes only a variadic list of (generally non-concatenated) !sim.fstring values .* It is expected to be placed within a procedural region that provides the control flow.

The design of the sim.print operation is aligned with other synchronous side-effecting operations, like sim.finish and verif.clocked_assert. This should make lowering to it fairly easy and be suitable for dataflow-ish analyses and transformations. On the other hand, the sim.proc.print operation can be mapped directly to the respective function calls in both the SV and Arc/LLVM backends.

There isn't much logic to the operations themselves. The major part of this PR is the new "ProceduralizeSim" pass which performs the transformation from sim.print to sim.proc.print. To do so it groups the print operations by their clock signal, flattens their format string arguments to a list of primitive tokens, and creates the procedural print operations wrapped within an (isolated from above) hw.triggered op for the clock and a scf.if op for the condition.

A potentially contentious point is ordering within non-procedural regions. I don't know if there is a consensus on how much, if at all, order matters in the body region of a HWModule. At first I considered giving the sim.print op an "artificial" argument and result to enforce a specific print order through a def-use-chain. But this felt like unnecessary complexity and I just went for order of occurrence for now.

Another problem is that there does not seem to be a cross-dialect notion of procedural vs. non-procedural regions. This is not the same as SSACFG region vs. graph region, is it? There is a ProceduralRegion OpTrait, but it is exclusive to the SV dialect. In practice this creates a bunch of ugly dependencies in the sim.proc.print verify method, as I need to explicitly list the legal parent ops. I'm tempted to just remove the verifier. *

*UPDATE: Changed sim.proc.print to take a single !sim.fstring argument, just like sim.print. I've also changed the verifier of the procedural print op. Since there is no good way to do an exhaustive check right now, it now only checks for parents that are definitely known to be non-procedural. In all uncertain cases it passes, to hopefully make lowering less of a hassle.

@fzi-hielscher fzi-hielscher marked this pull request as ready for review July 9, 2024 18:23
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really nice! Minor comment on the variadic-ness.

I like following the established pattern with a clock and condition operand. There has been ongoing discussion about how we could hopefully refactor that at some point such that the clock/condition can be extracted into a separate op that composes with other ops. Keeping all new ops in this established pattern will make it easy to change things around wholesale in the future once we settle on a design.

This operation must be within a procedural region.
}];

let arguments = (ins Variadic<FormatStringType>:$inputs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer sim.proc.print to take a single string input. Basically sim.print but without the clock and condition. That feels like a simpler op with one single task; with a variadic list of operands, the op would have to explain how it concatenates the operands, or which one of them it prints, etc. Since we have an explicit sim.fmt.concat, replicating part of that functionality in sim.proc.print doesn't feel necessary.

@fabianschuiki
Copy link
Contributor

At some point we'll have to teach arcilator about hw.triggered and all these nice ops 😏

@fzi-hielscher
Copy link
Contributor Author

Yes, we should totally do that. 😁
I did take a bunch shortcuts over there. Arguably, making sim.proc.print variadic is one of them. We have to flatten the format string for both the Verilog and Arcilator lowering path. So doing it right before lowering felt like an easy way to solve that. But you are right, it is conceptually quirky.

I'll try to factor it out so flattening can be done in the respective backend conversions without too much duplicated logic. It's just that I've grown slightly weary of the dialect conversion framework after reading this thread. Especially the

Walking IR and/or SSA use-def chains is generally unsafe in a ConversionRewritePattern [...]

part. I kind of wish I had never seen that... 😅 I suppose it is not going to be a problem in this case.

Thanks a lot for the review!

@fabianschuiki
Copy link
Contributor

Yes, we should totally do that. 😁

Really cool 😎.

Yeah maybe the lowering will have to go through something besides the dialect conversion framework. This feels like matching on the print op plus any of the format string ops feeding into the input to produce one singular SV format string (in the SV case). For arcilator we could probably call the corresponding printing functions directly 🤔.

The various restrictions about use-def chain walking make me nervous. It feels like the most interesting lowerings are the ones that match on multiple ops… 1-1 op lowerings feel overly simplistic.

@uenoku
Copy link
Member

uenoku commented Jul 10, 2024

A potentially contentious point is ordering within non-procedural regions. I don't know if there is a consensus on how much, if at all, order matters in the body region of a HWModule. At first I considered giving the sim.print op an "artificial" argument and result to enforce a specific print order through a def-use-chain. But this felt like unnecessary complexity and I just went for order of occurrence for now.

Yes, we really don't have a notion of ordering in non-procedural regions. When we introduced DPI function calls, we internally had a discussion on "artificial" argument and result (we called this token passing) but we didn't go that direction since it's complicated and invasive change.

a cross-dialect notion of procedural vs. non-procedural regions. This is not the same as SSACFG region vs. graph region, is it?

Yeah, since we don't have ordering in core dialect we don' have representation for procedural stuff in core (except for hw.triggered? anyway hw.triggered doesn't have any result now). Not having procedural stuff has kept core dialect representation dataflow-ish so not sure we want to break invariant for core dialects.

@fzi-hielscher fzi-hielscher merged commit 458717b into llvm:main Jul 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants