$print internal cell, for formatting tasks

[kivikakk]

This is a rebase of @whitequark's #2459; so far this is back to the same stage it was at. I'll continue down the checklist from here.

---

• Add tests (diff against iverilog) for the Fmt::render facility used by Yosys to print messages when it encounters initial $display statements and for $sformatf.
• Add tests (roundtrip identity check) for the Verilog/RTLIL translation facilities used to support the $print cell.
• Document the $print cell and its format string mini-language.
• Implement $print cell handling in CXXRTL.
• Add back %m support that existed in the old $display implementation (as an alias of %l) but was removed because it is unclear whether this implementation is correct.
• Add $time and %t support to format strings.
• Consider CXXRTL RFE: better debug messages for out-of-bound memory access #2520.
• Maybe fuzz the division procedures ported to CXXRTL value.

[kivikakk]

First pass of docs are complete; they can be seen rendered here: https://kivikakk.github.io/yosys/CHAPTER_CellLib.html#debugging-cells

[whitequark]

These are really nice!

[kivikakk]

I've now pushed a first pass at the CXXRTL implementation. Unordered notes:

• I'm not at all attached to the method of formatting output on the target side (i.e. template<size_t Bits> struct value_formatted), but it works reasonably well. I previously made an attempt outputting formatting specifiers from Fmt::emit_cxxrtl directly, but it was pretty ugly and half-baked. Open to any suggestions at making this cleaner — my C++ isn't super up-to-date and there might be much I'm missing.
• I've opted to depend on the end-user including <iostream> before including a design with any $print cells. We may want to change this, and we almost certainly want to allow configuring the output stream, currently hard-coded to std::cout.
• I was really pleased at how easy this was to put together despite having no prior experience with CXXRTL. The designs of both the $print cell and CxxrtlWorker made it trivial.
  • This is not to say I'm not missing anything, but so far testing various combinations of triggers, conditions, etc. all look good.
• I still need to check over the decimal formatting logic, particularly for signed values.
  • The fastest path to get arbitrary-precision decimal appeared to be translating BigUnsigned/BigInteger's division routines for use in the CXXRTL target environment. It seems to work OK, but I'll have to verify it a lot more before I can feel confident the translation didn't introduce any bugs, subtle or otherwise. The algorithms depends on some invariants that don't hold for value (mostly around BigUnsigned::len), so we have to emulate it a bit.

edit: not translating all of BigUnsigned/BigInteger, just their division routines.

[kivikakk]

Re: %m, assume a design like this:

module top;
    mid mid_uut ();
endmodule

module mid;
    bot bot_uut ();
endmodule

module bot;
    initial $display("%%l: %l\n%%m: %m");
endmodule

Icarus Verilog gives:

WARNING: display_m.v:10: %l currently unsupported $display<%l>.
%l: <%l>
%m: top.mid_uut.bot_uut

Verilator gives:

%l: bot
%m: TOP.top.mid_uut.bot_uut

We're currently showing \bot for %l, which seems fine. I'll go for a %m implementation similar to the others.

[kivikakk]

Okay, I think I'm essentially done. I've opted not to reach for #2520 at this time, just so I don't bite off more than I can chew right now. Misc notes follow:

• After discussion with @whitequark, I've left %m as equivalent to %l.
• I've fuzzed CXXRTL's ported division routine enough to feel confident in it. I've included the fuzzing harness in this PR, though I'm not sure it really fits into this repo. It depends on a clone of google/fuzztest in the same directory, which I hesitate to add to .gitmodules since there are no submodules otherwise.
• %t and $time/$realtime are now partly supported.
  • Other implementations let you use non-%t specifiers with $time and $realtime (e.g. %g), but we don't yet.
  • The CXXRTL backend also supports these, using the step() count for their value.
• write_cxxrtl got an option to change the output stream.
• fmt's tests compile and run CXXRTL output -- I'm just using ${CXX:-g++} to pick a compiler here, which doesn't respect the top-level Makefile configured compiler.

[mwkmwkmwk]

please also add the new cell to techlibs/common/simlib.v; preferably we'd have a working implementation that iverilog can execute, but if that's infeasible (and it looks like it could be), at the very list it should have a blackbox with the right parameters and ports

[mwkmwkmwk]

Unless I'm missing something, I also think the lzero flag is redundant and the cell design can be easily simplified by removing it from the IR — AFAICS it can be simply lowered in the Verilog-to-cell conversion by setting padding to '0', width to the computed max width of the signal, and if the format already had a larger width set, manually emitting the extra padding into the format string?

[mwkmwkmwk]

The cell in synchronous configuration seems quite reasonable to me (multiple triggers are a little weird though I see why it's done this way), but I think the async version semantics could use some discussion.

The documentation mentions "printing at every step", which is ill-defined for many backends, and doesn't match the behavior of the source Verilog (which prints whenever any input to the containing comb process changes) nor Verilog backend output (which prints on any change of the format argument or the enable signal). While the documented semantics are implemented as-is in CXXRTL, this is rather suboptimal when one wants to watch a combinatorial signal that changes rarely.

My proposal would be to define the cell to have the same behavior as what the Verilog backend currently emits: an async print cell prints whenever there's a change on ARG or EN signal and the EN signal is 1. On the CXXRTL side, that'd be realized by adding a variable to store the last observed value.

An alternative would be to fully implement source Verilog behavior and track down all triggers of the containing process, but this doesn't seem that useful, and is rather unwieldy to implement.

Also to be considered before the cell design is finalized: how about putting a PRIORITY parameter on the cell, for ordered output? As it stands, we have no way to preserve the output order when someone puts multiple $display calls in one sync process. This would unfortunately require some code to merge $print cells with the same trigger into a single process in Verilog backend, but I think it'd be a good usability improvement.

[kivikakk]

please also add the new cell to techlibs/common/simlib.v; preferably we'd have a working implementation that iverilog can execute, but if that's infeasible (and it looks like it could be), at the very list it should have a blackbox with the right parameters and ports

👍 It's not hard to generate the correct blocks for the triggers, but transforming the RTLIL format string to a Verilog one here would be tricky. I've added the blackbox for now.

[mwkmwkmwk]

please also add the new cell to techlibs/common/simlib.v; preferably we'd have a working implementation that iverilog can execute, but if that's infeasible (and it looks like it could be), at the very list it should have a blackbox with the right parameters and ports

+1 It's not hard to generate the correct blocks for the triggers, but transforming the RTLIL format string to a Verilog one here would be tricky. I've added the blackbox for now.

Yeah, having thought about it a bit, implemeting this cell here would be theoretically possible but incredibly unwieldy; let's just go with the blackbox and be done with it.

[kivikakk]

AFAICS it can be simply lowered in the Verilog-to-cell conversion by setting padding to '0', width to the computed max width of the signal, and if the format already had a larger width set, manually emitting the extra padding into the format string?

Nice -- this keeps the logic for handling this case in one place, at parse time where it belongs. (96a766e)

[kivikakk]

The documentation mentions "printing at every step", which is ill-defined for many backends, and doesn't match the behavior of the source Verilog (which prints whenever any input to the containing comb process changes) nor Verilog backend output (which prints on any change of the format argument or the enable signal). While the documented semantics are implemented as-is in CXXRTL, this is rather suboptimal when one wants to watch a combinatorial signal that changes rarely.

My proposal would be to define the cell to have the same behavior as what the Verilog backend currently emits: an async print cell prints whenever there's a change on ARG or EN signal and the EN signal is 1. On the CXXRTL side, that'd be realized by adding a variable to store the last observed value.

I agree! I wasn't quite sure what the behaviour of the $print cell should be when not triggered, and took the shortest path to the CXXRTL implementation, which happened to produce correct/useful sync behaviour, and the async behaviour we have now.

Having played around with iverilog's behaviour with $display in comb processes now, this seems straightforward.

An alternative would be to fully implement source Verilog behavior and track down all triggers of the containing process, but this doesn't seem that useful, and is rather unwieldy to implement.

Right, yes.

For my curiosity and poking around, what's the most straightforward way to simulate source Verilog behaviour? Cursory testing shows that Verilator does indeed seem to by default -- $displays in a comb cell are triggered (subject to conditions) on every change of cell input, regardless of which inputs feature in the $display parameter list.

iverilog appears to only trigger on input/condition changes. I also note iverilog triggers for each change during comb propagation (? terminology?); Verilator rolls them up.

Also to be considered before the cell design is finalized: how about putting a PRIORITY parameter on the cell, for ordered output? As it stands, we have no way to preserve the output order when someone puts multiple $display calls in one sync process. This would unfortunately require some code to merge $print cells with the same trigger into a single process in Verilog backend, but I think it'd be a good usability improvement.

This would at the very least simplify this test case a great deal!

[whitequark]

My proposal would be to define the cell to have the same behavior as what the Verilog backend currently emits: an async print cell prints whenever there's a change on ARG or EN signal and the EN signal is 1. On the CXXRTL side, that'd be realized by adding a variable to store the last observed value.

This is actually my original intent for the CXXRTL implementation.

Also to be considered before the cell design is finalized: how about putting a PRIORITY parameter on the cell, for ordered output?

No objection.

[kivikakk]

Also to be considered before the cell design is finalized: how about putting a PRIORITY parameter on the cell, for ordered output? As it stands, we have no way to preserve the output order when someone puts multiple $display calls in one sync process. This would unfortunately require some code to merge $print cells with the same trigger into a single process in Verilog backend, but I think it'd be a good usability improvement.

I was thinking about the possibility of doing this in a pass (i.e. grouping by trigger and then concat format strings + args in priority order), to avoid complicating the backend, but this falls flat in the face of differing enable conditions. I'm also not sure if there are Verilog implementations with restrictions on system task arg counts to worry about.

[mwkmwkmwk]

Also to be considered before the cell design is finalized: how about putting a PRIORITY parameter on the cell, for ordered output? As it stands, we have no way to preserve the output order when someone puts multiple $display calls in one sync process. This would unfortunately require some code to merge $print cells with the same trigger into a single process in Verilog backend, but I think it'd be a good usability improvement.

I was thinking about the possibility of doing this in a pass (i.e. grouping by trigger and then concat format strings + args in priority order), to avoid complicating the backend, but this falls flat in the face of differing enable conditions. I'm also not sure if there are Verilog implementations with restrictions on system task arg counts to worry about.

An alternative would be to create a print cell per process, have multiple enable inputs, and have some marker in the format string for "switch to the next enable in the list" (and presumably to a new line in the output); as a bonus, that'd ensure sane semantics in the async version.

I'm not sure if I like this version, it's getting a bit complex; what do you think?

[kivikakk]

I'm not sure if I like this version, it's getting a bit complex; what do you think?

I'm inclined to agree -- once the format string syntax starts getting stateful actions, it feels like it might grow a Common Lisp implementation when we're not looking. I don't think it's bad, but we can do well enough without the complexity imo.

Having actually made the backend change (still local WIP), it's not so bad as I imagined, so I think we can leave the pass idea alone for now.

[kivikakk]

OK!

• PRIORITY has been added, documented and implemented in the backends. We now have additional handling for sync $print cells, grouping them by trigger:
  • Verilog backend: we group them in dump_module.
  • CXXRTL backend: we group them into PRINT_SYNC flow graph nodes in analyze; it seemed neater than scanning all the CELL_EVALs in dump_eval_method and doing the grouping there.
  • The actual grouping is not intelligent: by std::pair of the trigger port SigSpec and trigger polarity bitfield. It seems sufficient.
• Comb $print cells in CXXRTL now fire only when {EN,ARGS} has changed since last eval() (subject to EN).
  • I've just added a value to the enclosing module per $print cell which in we cache/compare {EN,ARGS}.
  • I've done no checking or testing about how this behaves with multiple delta cycles, but presume $prints can trigger multiple times per step if {EN,ARGS} changes repeatedly during propagation.

It's also worth noting that Actions is triggering on my fork, so you can see early CI results: https://github.com/kivikakk/yosys/actions?query=branch%3Aprint-cell.

[povik]

(I gave this a spin in replacing a downstream solution for print cells I had, it works great, only thing I ran into is missing #include <iostream> in the generated CXXRTL code.)

[jix]

I'm interested in extending this in the future to support debug printing within SBY. This is what I'd eventually like to have:

• Support $print cells in yosys's sim command, including capturing the output data (with timestamps and hierarchy/src locations) in the optional JSON -summary output that SBY already uses when configured to use sim for trace generation.
• Parse assert (...) else $error(...); etc. in yosys's verilog frontend and generate $print cells for that.

It might be quite a while until I find time to work on this, (I also haven't gotten around to look at this PR in any detail yet), but I wanted to mention this now, in case anyone else is interested in working on this before I get to it or in case it's relevant for anything already in this PR.

[kivikakk]

only thing I ran into is missing #include <iostream> in the generated CXXRTL code

Ah, yep — seeing as the output stream now must be one of std::cout or std::cerr, there's little reason to not always #include <iostream>.

[whitequark]

FYI I'm planning to give this a proper review sometime next week, hopefully Mon-Tue.

[povik]

FWIW I pushed a rebase of this series below (there was slight conflict with #3722)
https://github.com/povik/yosys/commits/print-cell

[whitequark]

Absolutely fantastic work by everyone involved! This will be present in Yosys 0.33 release.

[povik]

Just in case this wasn't intentional let me point out this has gone in without the change to #include <iostream> automatically. (So commit 2e16f91 seems to have been dropped at some point along the way.)

[whitequark]

I suspect it wasn't intentional. @mwkmwkmwk could you take a look?

[mwkmwkmwk]

Indeed, it has been removed in an earlier force-push on 13.05.2023; see the comparison linked above: https://github.com/YosysHQ/yosys/compare/2e16f9192401b5c83ce18e8651d57a4fd69c45c0..52a7bec44c23a3c6124ac1893705419308ba0c82

[mwkmwkmwk]

This seems to be the only commit that has been lost in that rebase. I've just cherry-picked it onto master.