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

Restore sim output from initial $display #4131

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

daglem
Copy link
Contributor

@daglem daglem commented Jan 14, 2024

Restores sim output from initial $display, which was inadvertedly removed in #4129.

@daglem
Copy link
Contributor Author

daglem commented Jan 14, 2024

@whitequark Is this the correct fix?

@daglem
Copy link
Contributor Author

daglem commented Jan 14, 2024

The fix is based on the amended documentation for \TRG in 1159e48
"If the width of this port is zero and \TRG_ENABLE is true, the cell is triggered during initial evaluation (time zero) only."

@daglem
Copy link
Contributor Author

daglem commented Jan 14, 2024

FYI no ill effects in sv-tests either.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Actually, no; this would make the $print cell trigger on each cycle of sim, not just the initial one.

@daglem
Copy link
Contributor Author

daglem commented Jan 14, 2024

Actually, no; this would make the $print cell trigger on each cycle of sim, not just the initial one.

As far as I can tell, it works as intended. E.g. running ./yosys -p "read_verilog -nodisplay -sv high.sv; proc; sim" on this input

module top ();

bit [7:0] arr;

initial begin
	$display(":assert: (%d == 8)", $size(arr));
end

endmodule

yields

3. Executing SIM pass (simulate the circuit).
Simulating cycle 0.
:assert: (          8 == 8)
Simulating cycle 1.
Simulating cycle 2.
Simulating cycle 3.
Simulating cycle 4.
Simulating cycle 5.
Simulating cycle 6.
Simulating cycle 7.
Simulating cycle 8.
Simulating cycle 9.
Simulating cycle 10.
Simulating cycle 11.
Simulating cycle 12.
Simulating cycle 13.
Simulating cycle 14.
Simulating cycle 15.
Simulating cycle 16.
Simulating cycle 17.
Simulating cycle 18.
Simulating cycle 19.
Simulating cycle 20.
Simulating cycle 21.
Simulating cycle 22.
Simulating cycle 23.
Simulating cycle 24.
Simulating cycle 25.
Simulating cycle 26.
Simulating cycle 27.
Simulating cycle 28.
Simulating cycle 29.
Simulating cycle 30.
Simulating cycle 31.
Simulating cycle 32.
Simulating cycle 33.
Simulating cycle 34.
Simulating cycle 35.
Simulating cycle 36.
Simulating cycle 37.
Simulating cycle 38.
Simulating cycle 39.
Simulating cycle 40.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

OK. I don't understand the sim pass very well.

@povik
Copy link
Member

povik commented Jan 14, 2024

Actually, no; this would make the $print cell trigger on each cycle of sim, not just the initial one.

Well, on each cycle in which the arguments change, since the initial prints are now being handled as if TRG_ENABLE was false, so as an always @(*) $display.

Consider

module top(input wire clk);
reg [7:0] t = 0;
always @(posedge clk) t <= t + 1;
initial begin
	$display("%x", t);
end
endmodule

simulated with ./yosys -p "read_verilog -nodisplay test.v; proc; sim -clock clk".

You get

3. Executing SIM pass (simulate the circuit).
Simulating cycle 0.
00
Simulating cycle 1.
Simulating cycle 2.
01
Simulating cycle 3.
Simulating cycle 4.
02
Simulating cycle 5.
Simulating cycle 6.
03
Simulating cycle 7.
Simulating cycle 8.
...

@whitequark whitequark self-requested a review January 14, 2024 13:43
@daglem daglem force-pushed the fix-initial-display branch from f41822e to cc4506a Compare January 14, 2024 14:22
@daglem
Copy link
Contributor Author

daglem commented Jan 14, 2024

Actually, no; this would make the $print cell trigger on each cycle of sim, not just the initial one.

Well, on each cycle in which the arguments change, since the initial prints are now being handled as if TRG_ENABLE was false, so as an always @(*) $display.

@povik I tried to remedy this now - can you please have a look?

@povik
Copy link
Member

povik commented Jan 14, 2024

Still doesn't do what we want in all cases, consider:

module top(input wire clk);
reg [7:0] t = 0;
always @(posedge clk) t <= t + 1;
initial begin
	if (t % 2)
		$display("%x", t);
end
endmodule

this will make the EN port of the $print cell change value, which will repeatedly trigger the cell.

@povik
Copy link
Member

povik commented Jan 14, 2024

I suggest we establish a general way to query if we are in the initial cycle of a simulation. As far as I can tell, there's the set_initstate_outputs method that's already called to set whether the next cycle is the initial one. Maybe we should generalize that, add a method e.g. set_cycle_initial which in turn calls set_initstate_outputs and updates the "is initial cycle" flag. We would then change the existing call sites from set_initstate_outputs to set_cycle_initial.

What do you say @jix? Are we free to conflate the $initstate output value with a flag that says "this is the initial cycle of the simulation" which would influence things like the print cells?

@daglem daglem force-pushed the fix-initial-display branch from cc4506a to acf916f Compare January 14, 2024 15:52
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

I think we should eventually clean this up so that each print cell doesn't maintain its own ad hoc "initial" flag.

@daglem
Copy link
Contributor Author

daglem commented Jan 14, 2024

I think we should eventually clean this up so that each print cell doesn't maintain it's own ad hoc "initial" flag.

I was just about to write that I propose this as an interim solution 😅
All the cases above work now - it would be nice if this could be merged while you discuss other possibilites.

@povik
Copy link
Member

povik commented Jan 14, 2024

All the cases above work now - it would be nice if this could be merged while you discuss other possibilites.

I agree. Let's wait for CI to be happy then merge.

@povik povik merged commit 7281107 into YosysHQ:master Jan 14, 2024
16 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