Functions should avoid non-local references

[sifferman]

Hello. I'm a huge fan of this style guide, and I enforce it for all my students. Thank you!

In an Icarus issue (steveicarus/iverilog#983), we agreed that using continuous assignment from functions is inconsistent among tools and may deserve its own warning. I don't believe that the lowRISC style guide mentions this issue, so I wrote up a section on it.

[nbdd0121]

I think the fact that functions reference anything non-local is bad.

The whole "continuous assignment from function" issue is moot if we just disallow the function from referencing anything non-local.

[rswarbrick]

Gary has commented already, but I thought it might be worth adding a bit of background. Firstly, thank you very much for the contribution! It's really wonderful for us to be having an impact like you describe.

About the technical side of things, I think @nbdd0121 is probably right: we'll end up with a simpler style guide if we just tell people not to have a function that refers to anything that isn't an argument. This will also have the property you want, and disallows other things that are probably not a great idea.

Would you be happy to put together a version like that? We'd be very grateful!

[sifferman]

Thanks for the feedback! Yes, it seems that openTitan, Ibex, and CVA6 already follow that stricter rule.

I did find one exception in Ibex, but it is for DV, so it probably doesn't matter:
https://github.com/lowRISC/ibex/blob/f60d03b6b064c38e111931ff9b0bae70d6af9af1/examples/simple_system/rtl/ibex_simple_system.sv#L323-L325

[sifferman]

How does this look?

[GregAC]

I did find one exception in Ibex, but it is for DV, so it probably doesn't matter:
https://github.com/lowRISC/ibex/blob/f60d03b6b064c38e111931ff9b0bae70d6af9af1/examples/simple_system/rtl/ibex_simple_system.sv#L323-L325

Yup that's fine as it's not used by synthesisable code. It exists to give the testbench an easy way to read out performance counters.

[marnovandermaas]

Thanks for making these changes, I think this is a really valuable addition to the style guide. I just added a few minor comments. Would you mind squashing your commits into one commit? If you don't know how to do that, I can do it for you.

[sifferman]

Just squashed. Thanks for all the feedback!

Although I wanted to get your opinion on another issue with functions. VCS will currently not run this correctly:

module test;

function automatic [7:0] test_func;
    test_func = 1;
endfunction

wire [7:0] test_wire = test_func();

initial begin
    #1;
    $display("Expected %h, Recieved %h", test_func(), test_wire);
    // Will fail because test_func() is never run
    $finish;
end

endmodule

Maybe this should be a separate issue, but I wonder if there should be another rule against blanket calling of functions from assign/wire. It works fine if always_comb is used.

[rswarbrick]

This looks excellent! Thank you very much for the improvement.

[marnovandermaas]

Thanks for making these changes!

[rswarbrick]

Following up on the question about functions and continuous assignments: I didn't know that VCS wasn't happy with that! I just looked at IEEE 1800, and there's no mention of a constraint: the right hand side of a "net declaration assignment" seems to be an arbitrary expression in the syntax, at least.

If we want to think carefully about style guide recommendations for this, maybe we should start a separate issue.

[marnovandermaas]

Maybe this should be a separate issue, but I wonder if there should be another rule against blanket calling of functions from assign/wire. It works fine if always_comb is used.

We can probably add this in a separate PR, but it sounds like we should restrict assigning a functions output directly to a wire and mandate that this happens in an always_comb block.

[GregAC]

I think I'd prefer to outright ban wire [7:0] test = test_func() type statements. This is because their semantics varies depending upon the net type. For wire you do get the equivilent to a continuous assignment but for logic it's just an initial value setting (much like an initial block though not identical! Those initial assignments are made before any initial blocks are run). Switching wire to logic may otherwise appear innocuous so better to avoid the complexity altogether. I note we explicitly allowed this currently:

style-guides/VerilogCodingStyle.md

Lines 2702 to 2725 in 5cd912c

	It is permissible to use wire as a short-hand to both declare a net and perform
	continuous assignment. Take care not to confuse continuous assignment with
	initialization. For example:
	👍
	```systemverilog {.good}
	wire [7:0] sum = a + b; // Continuous assignment
	```
	👎
	```systemverilog {.bad}
	logic [7:0] sum = a + b; // Initialization (not synthesizable)
	```
	`sum` is initialized to sum of initial values of a and b.
	👍
	```systemverilog {.good}
	logic [7:0] acc = '0; // Initialization (synthesizable on some FPGA tools)
	```
	There are exceptions for places where `logic` is inappropriate. For example,
	nets that connect to bidirectional (`inout`) ports must be declared with `wire`.
	These exceptions should be justified with a short comment.

Initial assignments can be useful in FPGA but are deadly for ASIC synthesis as it's a great way to get simulation/synthesis mismatch where the FPGA version agrees with the simulation but the ASIC synthesis doesn't. If you confine them to an 'initial block at least it makes it far obvious you have one. We could consider saying initial blocks are only allowed in RTL that is strictly for FPGA usage (e.g. top-levels and wrappers etc that we may use on FPGA to instantiate IP)

[nbdd0121]

I personally don't think we should get rid of the wire shorthand. It is orthogonal to the reported VCS issue.

The VCS issue probably is caused by the function being zero argument (and therefore not sensitive to anything). I think we should ban functions with no input instead. It could be a localparam instead.

[a-will]

I think I'd prefer to outright ban wire [7:0] test = test_func() type statements. This is because their semantics varies depending upon the net type. For wire you do get the equivilent to a continuous assignment but for logic it's just an initial value setting (much like an initial block though not identical! Those initial assignments are made before any initial blocks are run). Switching wire to logic may otherwise appear innocuous so better to avoid the complexity altogether. I note we explicitly allowed this currently:

(pedantic mode on) wire is a "net type" and logic is a "data type". Two different things. This is frequently taught incorrectly in universities, and it's a pet peeve of mine.

Unfortunately, because many, many people have received the same poor instruction, that logic should just be used everywhere and can replace the wire vs. reg confusion, we're left with a lot of bad practices and more confusion. The differences between nets and variables are still relevant.

This language maybe gets a little too cute with defaults:

// Equivalent variable declarations with initial values.
logic foo = bar;
var logic foo = bar;

// Equivalent net declarations with continuous assignment.
wire foo = bar;
wire logic foo = bar;

[sifferman]

I just created a new issue for this. We can discuss more here: #67