lowRISC / style-guides

lowRISC Style Guides
Creative Commons Attribution 4.0 International
370 stars 123 forks source link

Continuous assignment from argumentless functions #67

Open sifferman opened 1 year ago

sifferman commented 1 year ago

This is a dedicated location to discuss a potential addition to the style guide to cover the following edge case:

VCS (2020.12) 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

Possible solutions

This is a summary of the discussion made in #66.

Full explanation by @GregAC in https://github.com/lowRISC/style-guides/issues/66#issuecomment-1662118280

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:

https://github.com/lowRISC/style-guides/blob/5cd912c617c712c773fdc9da9259298d183a2da9/VerilogCodingStyle.md?plain=1#L2702-L2725

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)

sifferman commented 1 year ago

@nbdd0121 Disallowing argumentless functions would work. I don't see many people using them. It seems the best use-case is just for style consistency for a group of related functions.

For example: https://github.com/pulp-platform/riscv-dbg/blob/138d74bcaa90c70180c12215db3776813d2a95f2/src/dm_pkg.sv#L420-L437

 function automatic logic [31:0] csrr (csr_reg_t   csr,
                                        logic [4:0] dest);
    // rs1, CSRRS, rd, OpCode System
    return {csr, 5'h0, 3'h2, dest, 7'h73};
  endfunction

  function automatic logic [31:0] branch(logic [4:0]  src2,
                                         logic [4:0]  src1,
                                         logic [2:0]  funct3,
                                         logic [11:0] offset);
    // OpCode Branch
    return {offset[11], offset[9:4], src2, src1, funct3,
        offset[3:0], offset[10], 7'b11_000_11};
  endfunction

  function automatic logic [31:0] ebreak ();
    return 32'h00100073;
  endfunction
sifferman commented 1 year ago

By @a-will in https://github.com/lowRISC/style-guides/issues/66#issuecomment-1662542373

// 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;

Wow, I didn't know you could do this. I think this example should be added to the "Use logic for synthesis" section. It may help stress the importance of the rule a bit more