I'm not confident that this should be merged in its current form. I'm currently working on modifying the frontend to emit $check cells, and there is intersection with what this PR is doing (namely, generation of RTLIL for initial assert (...); and later conversion back to Verilog). I think $check and $print cells should be handled uniformly in this aspect.

I'm not confident that this should be merged in its current form. I'm currently working on modifying the frontend to emit $check cells, and there is intersection with what this PR is doing (namely, generation of RTLIL for initial assert (...); and later conversion back to Verilog). I think $check and $print cells should be handled uniformly in this aspect.

Since the changes in the frontend are so small, could you still consider merging this, and improve on it / rewrite it later?
I'd love to have this working now. And perfect is the enemy of good! 😉

Since the changes in the frontend are so small, could you still consider merging this, and improve on it / rewrite it later?

No. In fact, upon further review, I think #3963 has issues that should be resolved first.

Since the changes in the frontend are so small, could you still consider merging this, and improve on it / rewrite it later?

No. In fact, upon further review, I think #3963 has issues that should be resolved first.

Sure, but I cherry picked only the first two commits, not the third (unfinished) one. Are there really any controversial changes in the current PR?

Are there really any controversial changes in the current PR?

I thought there is at first, but on closer look it seems like the cherry picked commits are OK functionality-wise. If you rename the variable in 97d655a from required to may_fail I'm fine with having it.

4e4d702 should definitely not be merged.

For 0792744, I've added inline comments.

Note that this comment:

4e4d702 should definitely not be merged.

directly relates to this comment of mine:

I'm currently working on modifying the frontend to emit $check cells, and there is intersection with what this PR is doing (namely, generation of RTLIL for initial assert (...); and later conversion back to Verilog). I think $check and $print cells should be handled uniformly in this aspect.

So more work will need to be done on $print in initial blocks no matter what. (I'm OK merging this PR excluding the problematic commit in the meantime.)

I've narrowed down the cause of the issues with .TRG_POLARITY and currently working on a fix.

I've narrowed down the cause of the issues with .TRG_POLARITY and currently working on a fix.

Great!

You can cherry pick cfbaf18 to have a fix the .TRG_POLARITY issue in this PR.

I'm currently working on modifying the frontend to emit $check cells, and there is intersection with what this PR is doing (namely, generation of RTLIL for initial assert (...); and later conversion back to Verilog). I think $check and $print cells should be handled uniformly in this aspect.

There is actually still a bug in the Verilog backend even after my fix: this testcase

module string_format_top;
	parameter STR = "something interesting";
	initial begin
		$display("A: %s", STR);
		$display("B: %0s", STR);
	end
endmodule

is transformed via -b verilog to this code:

(* hdlname = "\\string_format_top" *)
(* top =  1  *)
(* src = "tests/simple/string_format.v:1.1-7.10" *)
module string_format_top();
  (* src = "tests/simple/string_format.v:4.3-4.26" *)
  wire _0_;
  (* src = "tests/simple/string_format.v:5.3-5.27" *)
  wire _1_;
  always @*
    if (1'h1)
      $write("A: %s\n", $unsigned(168'h736f6d657468696e6720696e746572657374696e67));
  always @*
    if (1'h1)
      $write("B: %s\n", $unsigned(168'h736f6d657468696e6720696e746572657374696e67));
  assign _1_ = 1'h1;
  assign _0_ = 1'h1;
endmodule

This is very obviously wrong. However, since this particular bit is entirely unrelated to what this PR is doing, it doesn't need to block this PR.

You can cherry pick cfbaf18 to have a fix the .TRG_POLARITY issue in this PR.

For some reason I'm not able to cherry pick that - is there some git incantation I have to cast? I have synced my local repository, and I was able to cherry pick the out-of-tree commits above earlier.
Edit: I think I remember what I did now 😅

Done! I've also removed my attempt at a fix.

I don't think you pushed it.

I don't think you pushed it.

Yeah, I just found out 😅

Please also cherry pick 5579330 since this fixes the bug I described in #4129 (comment).

Please also cherry pick 5579330 since this fixes the bug I described in #4129 (comment).

Great, done!

LGTM

Thanks for all the help!

BTW awesome work on the $print cell, this really brings major improvements to Yosys!

Thanks! It was motivated by Amaranth but I implemented it (or at least started that work) in a way that benefits everyone. I do have to say that implementing SystemVerilog semantics correctly took a lot of time and effort...

Thanks! It was motivated by Amaranth but I implemented it (or at least started that work) in a way that benefits everyone. I do have to say that implementing SystemVerilog semantics correctly took a lot of time and effort...

I can only imagine! Give yourself a pat on the back, knowing that this would quite possibly never have materialized without your efforts.

It seems like 5579330 causes tests/fmt/display_lm_tb.cc to fail.

That happens because CXXRTL does not (yet) support initial statements with code. It's not clear how or whether to implement that (it would have to be exposed to the record/replay code and the C interface, for one).

That happens because CXXRTL does not (yet) support initial statements with code. It's not clear how or whether to implement that (it would have to be exposed to the record/replay code and the C interface, for one).

Can I simply temporarily comment out the initial $display line in the test, seeing that CXXRTL isn't ready for that yet?
Or perhaps you'd rather amend your commit with that (the commenting out, I mean)?
Edit: It seems like the always $display line doesn't work either.

Edit: It seems like the always $display line doesn't work either.

That seems to be a bug in CXXRTL.

Actually no, it's not. This is the command that fails:

+ grep ^%l: \\bot$ yosys-display_lm.log

This is the testbench output:

$ ./tests/fmt/yosys-display_lm_cc 
%l: \bot
%m: \bot

I think it's a broken testbench. However, while looking at this, I did find a subtle bug in that CXXRTL code.

Please cherry-pick commit 388df43.

I think it's a broken testbench.

The testbench expects both the synthesis log and the simulation log to contain the output. By commenting out initial $display it is no longer contained in the synthesis log, so the test fails.

I think it's a broken testbench.

The testbench expects both the synthesis log and the simulation log to contain the output. By commenting out initial $display it is no longer contained in the synthesis log, so the test fails.

Yes, I just found a fix for this - both removing the initial line and changing a line in run-test.sh to
../../yosys -p "read_verilog display_lm.v; hierarchy -auto-top; proc; sim" >yosys-display_lm.log fixes it.

I'm not sure that's really the right fix? I'm actually looking at implementing initial $print in CXXRTL right now.

I'm not sure that's really the right fix? I'm actually looking at implementing initial $print in CXXRTL right now.

Oh, I didn't imagine you'd be attacking that immediately!
I'll just remove my fix then.

Cherry-pick 0dd0128.

Color me impressed! 😅

Please also cherry pick 5579330 since this fixes the bug I described in #4129 (comment).

@whitequark Whoops, this commit also completely removes initial $display output from simulation.

What is "simulation" here?

What is "simulation" here?

In this context, the sim command. See #4131