I recently observed that in the OpenTitan codebase, we are not using parameter or localparam consistently when it comes to constants that are defined inside a package, but that are used outside the package in the corresponding module.

For example, the auto-generated register packages such as aes_reg_pkg.sv define all constants as parameter as they are used mainly outside the package. In contrast, some manually-written packages like hmac_pkg.sv we use localparam for constants that are used outside the package but inside the hmac RTL.

Our style guide currently is ambiguous regarding this. On one hand it says in https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#constants:

When declaring a constant:

• within a package use parameter.
• within a module or class use localparam.

The preferred method of defining constants is to declare a package and declare all constants as a parameter within that package. If the constants are to be used in only one file, it is acceptable to keep them defined within that file rather than a separate package.

On the other hand, it also recommends in https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#parameterized-objects-modules-etc:

• Use parameter to parameterize, and localparam to declare module-scoped constants.
• Use package parameters to transmit global constants through a hierarchy instead of parameters. To declare a constant whose scope is internal to the module, use localparam instead.

To me, this guideline is quite clear when it comes to packages: use parameter for everything that is used also outside the package and use localparam only if the constant is used within the package exclusively. Or did I miss something?

Maybe we should clarify in the latter section of the style guide that by "module-scoped"/"module" the SystemVerilog module and not the IP core is meant. WDYT?

I don't think we currently have any guidance on the use of functions, in particular I think this is important for synthesizable RTL. DV code may want different guidelines or be happy to have none. I'm purely considering synthesizable RTL here.

A few questions to answer:

1. Do we allow use of functions at all in synthesisable RTL?
2. Do we allow use of local variables within functions?
3. Do we mandate use of 'automatic' in all function definitions

I think our answers should be, yes we allow functions, that can use local variables and must always be declared automatic. We probably need some details around how local variables can be used within functions

Clearly functions are useful so saying no functions at all seems very limiting. Allowing local variables though can open up problematic behaviour, e.g.

function [3:0] do_a_thing(input [3:0] foo);
  logic [3:0] state = 0;
  
  if (foo == ConstantValue) begin
    state = state + 1;
    return state;
  end else begin
    return foo;
  end
endfunction

By default state is static so this function has persistent state between calls. This has clear issues and shouldn't be allowed.

You can use the 'automatic' qualifier on a function to remove the default static state-holding behaviour I suspect there's some further gotchas around local variables in functions we need to be wary of.

I'd like to propose to prefer SystemVerilog-2017 instead of 2012.

Reasons are;

1. SystemVerilog-2017 is just a clarified version of SystemVerilog-2012.
2. Language Reference Manual (LRM) of SystemVerilog-2017 is available for download at no cost.

The current description;

L181

Prefer SystemVerilog-2012.

All RTL and tests should be developed in SystemVerilog, following the SystemVerilog-2012 standard, except for prohibited features.

As described in this blog, SystemVerilog 2017 has no new features. The only functional change is removal of the operator overloading. Other changes are clarifications.

We have a reference to SystemVerilog 2012 which costs $449.
On the other hand the SystemVerilog 2017 LRM is part of IEEE GET Program. Readers can download it at no cost.

Preferring SystemVerilog-2017 encourage readers to access genuine clarified LRM and let them know SystemVerilog is a stable standard.

If you agree, I will propose a pull-request.

Thanks.

The current Verilog style guide mandates ALL_CAPS for macro definitions:
https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#macro-definitions

IMO SV language already provides a semantic separation for macros through a preceding backtick. There doesn't seem to be a need to mimic C/C++ here. Most editors have SV syntax support so macros can be made out quite easily.

DV code has heavy use of macro calls defined in UVM library, and all of them are lower_snake_case. UVM follows the following convention:

Macros representing constants: ALL_CAPS

`define FOO_WIDTH 32

Macros used for conditional compile: ALL_CAPS

`ifdef GATESIM
  // compile GLS specific code
`endif

Macros used for shortening a snippet of code: lower_snake_case

`define get_parity(val, odd=0) (^val ^ odd)

  p = `get_parity(rdata, cfg.use_odd_parity);

If we are mandated to use ALL_CAPS, then DV code ends up becoming a mix of macros invocations from UVM, which are lowercase and macro invocations from our libraries, in ALL_CAPS. This ends up making the code look ugly and inconsistent.

I am inclined to follow the same convention as UVM, at least for DV. Seeking thoughts and opinions on this.

I have noticed that verilog passed parameters are recommended to be coded as camelcase style. I think this is bad idea. My only argument is that all capital letters is the defacto standard in the industry. Every style guideline that i have every read or created states this.

It is clear that this style is ubiquitous. I notice that under the heading of Parameterized Types in the verilog style guidelines, there are examples showing all capitial letter parameters there. Even if this issue is rejected, some work needs to be done to clean up the style guidelines in its present state.

Additionally, I believe this applies to both local parameters and emul types.

We should add a separate RTL style guide for security related applications. The actual contents of that style guide still need to be fleshed out, but some aspects that should be covered have surfaced in #12 (e.g., guidance on FSM coding and placing of security-related assertions).

case (1'b1) statements can be used as a replacement for a series of if/else blocks. It is essentially a style preference for some coders, but has the significant downside of miscompiling in Quartus Lite/Standard (and possibly other tools). Can we disallow or discourage its use?

See
https://github.com/MarekPikula/quartus-sv-gotchas/blob/master/Intel%20Quartus%20SystemVerilog%20gotchas.md#1252-constant-expression-in-case-statement

and https://github.com/lowRISC/ibex/pull/336/files for more details.

Bad:

unique case (1'b1)
  csr_save_if_i: begin
    exception_pc = pc_if_i;
  end
  csr_save_id_i: begin
    exception_pc = pc_id_i;
  end
  default:;
endcase

Good:

if (csr_save_if_i) begin
  exception_pc = pc_if_i;
end if (csr_save_id_i) begin
  exception_pc = pc_id_i;
end

@sjgitty @eunchan @sriyerg @NilsGraf Any strong feelings/thoughts?

The current proposals for removing the use of X from RTL (#12) focusses on cases where the generation of X would have been the error, so replacing the use of X with appropriate assertions and coding style changes is a sufficient replacement.

There are also areas where the X generation itself isn't because of an exceptional condition but rather flags a particular wire should have it's value ignored. So using the generated X value is the error you want to trap.

A typical example of this is on a bus interface. A valid signal accompanies some request information (address, type etc). When the valid signal is low the request information shouldn't be used and can be driven to X to signify this.

An example of a bug this can find is as follows (this could be a code fragment from a device acting on requests from the bus, NEVER_X is a macro that gives you an assert that fires if the specified signal becomes X)

...
input logic         req_valid_i
input logic [7:0] req_addr_i
...

assign action_addr = req_addr_i == `ADDR_ACTION;

// Bug here, should have & req_valid_i
assign action_en = action_addr_i;

`NEVER_X(action_en);

When req_addr_i is driven to X when req_valid_i is low the assertion will quickly fire and the bug found an fixed. Where X is not used and the RTL doesn't tend to generate (ADDR_ACTION) as a value for req_addr_i when req_valid_i is low.

We can have explicit X driving in the DV environment on specific buses to help recreate this but you'd still want the NEVER_X macro or similar to ensure you get a quick, easy to diagnose failure. Plus you may want similar checking on internal signals that aren't well defined interfaces the DV can probe/drive values on (e.g. in Ibex we could set the write address of the register file to X when we're not writing a register).

One replacement would be for `NEVER_X(action_en) is be replaced by an assertion action_en -> req_valid_i. In the style guide we would recommend any 'enabling' signal that triggers an action has an assertion that checks if it is set the appropriate 'upstream enabling' signal is also set.

You can flip the above example on its head and instead have a bug where req_valid_i is being asserted when there is no valid request only this time we've fixed the bug above.

assign action_en = action_addr_i & req_valid_i;

If the RTL generating the address drives X on address when it's not intended to produce a request (but the RTL that generates the valid signal has a bug and sets valid anyway) again we'll quickly hit an easy to diagnose failure. Without the X if req_addr_i is mostly 0 when no request is meant to be generated (and a request against address 0 is a no-op) but occasionally is `ADDR_ACTION when no request is meant to be generated we've got a failure that we're less likely to see (and my be harder to diagnose when we see it) for the same bug.

I can't immediately think of a good replacement for this (whether that's an assertion or a coding style change). Though perhaps a bit of an artificial example we're not hugely worried about in practice?

Our current style guide does not appear to have a recommendation for whether we should add a space after wait and the opening parenthesis. It will be useful to settle on one so that Verible knows what to do.

Reason for adding space between wait and parenthesis:

• By following the Spaces around Keywords rule, the current rule set is simple - there is no need for exceptions

Reason for not adding space:

• It's an event control statement that behaves very much like a task, so it naturally aligns better with Function and Task Calls rule.
• Our current code base currently follows this style, so less churn

Interestingly, the LRM also has a mixed usage of both styles. Though, it is of course not the best place to look for style recommendations.

Thanks
Sri

I have read this part of the verilog style guidelines:
Logical vs. Bitwise
Use logical constructs for logical comparisons, bit-wise for data.

Logical constructs (!, ||, &&, ==, !=) should be used for all constructs that are evaluating logic (true or false) values, such as if clauses and ternary assignments. Use bit-wise constructs (~, |, &, ^) for all data constructs, even if scalar.

I reread the style guidelines again on this. I concluded that the guidelines could be roughly rewritten like this: "For control path statements, use logical operators. For data path statements, use bit wise operators, even if the data is a single bit."

So for control logic, these examples apply:

1. if (fifo_pop && enable[0]) then
2. assign fifo_push = (fifo_not_full && enable) ? fifo_write : 1'b0;
3. assign fifo_push = fifo_not_full && enable && fifo_write;

For data path:

1. assign write_bus[31:0] = raw_bus[31:0] & {32{write_valid}}; (vector case)
2. assign write_bus[0] = raw_bus[0] & write_valid; (scalar case)

This issue has been opened to clean up any ambiguities in the style guidelines.

The style guide currently recommends the following style for case statements

alway_comb begin
  unique case(in[3:0])
    4'b0001: out = 4'hE;
    4'b0110: out = 4'hA;
    4'b1000: out = 4'h6;
    default: out = 4'h0;
  end
end

Under standard system verilog semantics this blocks x propagation but due to the default: label we won't see any unexpected values in the output.

However if you turn on xprop in a simulator an x value on in results in an x on out (the simulation sees that out could change depending upon the value of in so as in is x it assigns x to out this is a change to the standard system verilog semantics).

This causes issues when ASSERT_KNOWN is used for values that are downstream of such a case statement (i.e. either out directly is used in an ASSERT_KNOWN or some signal derived from out is). When xprop is enabled the ASSERT_KNOWN is triggered.

A practical example of this can be found in the Ibex RTL, the alu_op_a_mux_sel must always be known:

https://github.com/lowRISC/ibex/blob/2c198383a363c6c8512d9e56fbee126fa74bbf04/rtl/ibex_id_stage.sv#L855

However it's generated via the (style-guide) compliant always_comb statement inside ibex_decoder:

https://github.com/lowRISC/ibex/blob/2c198383a363c6c8512d9e56fbee126fa74bbf04/rtl/ibex_decoder.sv#L543-L791

The decoder itself is fed directly with the instruction flop which will start the simulation as x. So when xprop is enabled the ASSERT_KNOWN for alu_op_a_mux_sel fails.

I can see a few possible solutions:

1. Declare that due to the rules in our style guide (with hopefully some lint to back them up!) that xprop is not a useful feature and shouldn't be used in simulations

2. Explain the xprop issue in the style guide so readers are aware. This may mean many of our unqualified ASSERT_KNOWN uses will require an enable term, we may want a new macro ASSERT_KNOWN_IF or similar for providing an enable term.

3. Alter the RTL so such signals get a suitable valid factored in so the x that gets propagated via the case or if/else if/else statements is squashed.

I don't think 3. is a good idea, so this leaves us with 1. or 2. I'd tend towards 1. but there may be good reasons this is a bad idea I haven't yet realised.

In the SystemVerilog style guide, should we require module instantiations which fit on a single line to do so? (We're trying to come down to a single way of formatting certain constructs, so this question is either-or.)

Option 1:

  and2 my_gate (.out_o(sig_output), .in1_i(first_input), .in2_i(second_input));

vs

Option 2:

  and2 my_gate (
    .out_o(sig_output),
    .in1_i(first_input),
    .in2_i(second_input)
  );

We already prefer (or allow) the use of Option 1 in other occasions, so it would be consistent with that (e.g. https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#begin--end).

I guess the main use case would be the instantiation of SV modules modeling gates, clock buffers, or other tiny modules.

@hzeller Is this something Verible has an opinion on right now?

CC @mdhayter @eunchan @msfschaffner @tomroberts-lowrisc @dsandrs @GregAC @rswarbrick @sriyerg @mwbranstad

(Split out from #48)

This could be a separate document, or could be a subsection of the Verilog Coding Style Guide. I recommend the latter, but it could be large enough to be independent.

Refers to #4

Hi,

I've been working on SystemVerilog Style Guide for my company with my colleagues.
I found your style guide and it is very well written. The goal of your stile guide and ours are very close, because we are also referring to books and papers by Stuart Sutherland, Don Mills, and Clifford Cumming as your Style Guide is.

I have some feedbacks and some proposals on your style guide.
Is your style guide only for lowRISC project?
I think your style guide can be applied to wide range of projects. If you don't mind I'd like to send pull requests to you.

Here is my fixes ready to be push.
master...hirooih:master

I have some other proposals. Some of them need Q&A with you. Some needs discussion on a separate issue ticket.
Before going to further let me know your thoughts.

Thank you sharing your great style guide.

Hi LowRISC team,

My name is Jonathan Mayer. I no longer work at or represent Google, but while I did I was the principal author Google's internal Verilog Style Guide. Especially going back in the change history -- I notice that you seem to have taken that style guide, made some minor changes to it, and published it as your own original work. While I appreciate that the style guide has finally seen light of day, that sort of seems like plagarism to me?

Has this document been released by Google under creative commons in some way that I'm not aware of? If so, why has the attribution of authorship been removed?

I don't represent Google, but I'd like to suggest that you work with Google to finally publish their original version of this style guide, and to fix the attribution?

Thank you,

• Jonathan.

The naming convention specifies module parameters should use the UpperCamelCase style:

But several examples in the guide inconsistently use the styling for constants:

The example contains a typo in the Suffixes section. The input signal clk_i is declared, but clk is used

module simple (
input clk_i,
input rst_ni, // Active low reset

// writer interface
input [15:0] data_i,
input valid_i,
output ready_o,

// bi-directional bus
inout [7:0] driver_io, // Bi directional signal

// Differential pair output
output lvds_po, // Positive part of the differential signal
output lvds_no // Negative part of the differential signal
);

logic valid_d, valid_q, valid_q2, valid_q3;
assign valid_d = valid_i; // next state assignment

always_ff @(posedge clk or negedge rst_ni) begin
if (!rst_ni) begin
valid_q <= '0;
valid_q2 <= '0;
valid_q3 <= '0;
end else begin
valid_q <= valid_d;
valid_q2 <= valid_q;
valid_q3 <= valid_q2;
end
end

assign ready_o = valid_q3; // three clock cycles delay

endmodule // simple

Our style guide currently has this example:

assign structure = '{
    src: src,
    dest: dest,
    default: '0};

Note the placement of the closing };.

In the wild, we have a fair bit of code which uses the following style:

assign structure = '{
    src: src,
    dest: dest,
    default: '0
};

(e.g. https://github.com/lowRISC/opentitan/blob/a4a9e4013612607c68086ce4534a14206928f3a6/hw/ip/prim/rtl/prim_keccak.sv#L191-L198)

Currently our style guide doesn't have language (beyond that example) mandating one use over the other. Which option do we prefer?

Hi,

Do you have any rules file defined with the lowRISC coding guidelines that I can run on Spyglass?

from #22 a discussion on where and how to use objections has been started.
to not clutter that PR too much I have taken discussion this into its own issue.

The original suggestion is to avoid objections in drivers and monitors and instead add them in the scoreboard.

I will try to make an argument why monitors is the perfect place for objections and why they should only go into phase_ready_to_end()

one of the Arguments of why it is nice to have objections in the scoreboard is given by @sriyerg
I took the liberty of quoting it here:

I see some valid usecases in allowing raising/dropping objections in the scoreboard; example: in xbar type DUT, raise obj when transaction enters the DUT and drop only when it exits. This objection (that helps ensure that the test does not exit prematurely) cannot be captured in the monitor and has to be captured in a higher level component. It cannot be captured in the test either since it only generates and sends C-R stimulus to the driver. It has no knowledge of when the transactions actually enter and exit the DUT.

We keep the performance impact low by raising objection exactly once (dont raise if it is already raised), which is what the line implies. You can find that logic in our base scoreboard (hw/dv/sv/dv_lib/dv_base_scoreboard.sv). If by deadlocks you mean hangs, then there is an obvious problem either in the design or dv, that needs fixing. We avoid hanging forever through reasonable test timeouts.

I have two arguments against this - the first one being simply performance.
every time you raise and drop an objection is causing the component hierarchy to be traversed.
so for shallow environments this might be a minor impact but as the hierarchy gets deeper this quickly gets expensive.

The other argument is that implementing this in the scoreboard requires a watchdog timer to be implemented a long the side to avoid deadlocks.

Here is what I suggest

Most modules have deterministic input to output latency and does not need any objections besides test case objections and a good choice of drain time. this should be the default go to.
objections should only be used outside the test when the DUT latency is not detministic!

Some modules does not have deterministic latency. an example can be some sort of packet builder where input packets are grouped in larger chunks and these chunks are only forwarded when reaching a certain size.

Modules like this usually have an internal timeout to flush out undersized chunks.

In such a scenario it will be hard to raise objections in a scoreboard as the scoreboard cannot easily predict the number of outputs based on the number of inputs.
and the suggested objection method would be insufficient.

instead I suggest putting the objetion inside the phase_ready_to_end() function.
this is called once all modules have dropped their run_phase objections and the test is trying to end the run phase.

here is what is currently in my scoreboard (AES)

 virtual function void phase_ready_to_end(uvm_phase phase);
    if (phase.get_name() != "run") return;
    if ( (dut_fifo.num() != 0 ) || (ref_fifo.num() != 0) ) begin
      phase.raise_objection(this, "fifos still has data");
      fork begin
        wait_fifo_empty();
        phase.drop_objection(this);
      end
      join_none
    end
  endfunction


  virtual task wait_fifo_empty();
    wait ((dut_fifo.num() == 0 ) && (ref_fifo.num() == 0));
  endtask

  // ...
endclass

This ensures the Objection is ever only raised once!
obviously this still has the deadlock issue so a time would need to be implemented in parallel to the ''' wait_for_fifo_empty ''' function.

But this is still sub-optimal because the scoreboard should be a 0-time and there for it does not have any knowledge of clock frequency and one would have to guess what number of milliseconds is enough for any used clock frequency.

I believe the better choice is to implement a function in the monitor that detects traffic and keeps the phase from ending for /# number of clocks.
this is very similar to drain time but where drain time is for the full environment this implementation will work even when multiple environments are cascaded to form a top level verification environment.

the idea is to start a timer when phase_ready_to_end is called.
if no traffic is seen before a timer runs out - it is ok to end.

here is an example I did

function void phase_ready_to_end(uvm_phase phase);
      uvm_phase current_phase;
      string str ="";
      uvm_object objectors[$];
      current_phase = phase;

      if (phase.is(uvm_main_phase::get())) begin       
      // Start watchdog
        fork
          watchdog_ok_to_end();
        join_none;
        if (!ok_to_end) begin
          phase.raise_objection(this);
          fork begin
            wait_for_ok_end();
            phase.drop_objection(this);

            // see who is still objecting
            current_phase.get_objection().get_objectors(objectors); // get all that has raised objections
            str = $sformatf("\n\n\t |--------------------| Who Is Still OBJECTNG |------------------------|"); 
            str = {str, $sformatf("\n\t  current phase: %s ", current_phase.get_name())};
            str = {str, $sformatf("\n\t Hierarchical Name                                              Class Type")};
            foreach(objectors[i]) begin
              str = {str, $sformatf("\n\t%-60s%s\n\n", objectors[i].get_full_name(), objectors[i].get_type_name())};
            end
            `uvm_info({LOG_NAME, "*"}, str, UVM_HIGH);
          end join_none;
        end
      end
    endfunction

    //-----------------------
    // Wait until ok_to_end is set. Only checks ok_to_end every timeout_delay cycles since this is the length of the watchdog cycle
    task wait_for_ok_end();
      forever begin
        repeat(timeout_delay) @(posedge vif.clk_i);
        if (ok_to_end) break;
      end
    endtask // wait_for_ok_end

 // Watchdog waits timeout_delay cycles while checking if there is any data on the interface. If there is then !ok_to_end else ok_to_end.
    task watchdog_ok_to_end();
      bit        reset_timer = 0;
      fork
        begin        
          forever begin         
            @(posedge vif.clk_i);
            if(vif.vif.rd) begin           
              reset_timer = 1;
            end          
          end
        end
        begin
          forever begin
            ok_to_end = 0;
            repeat(timeout_delay) @(posedge vif.clk_i); // stay here until #timeout delay clocks has passed
            if(!reset_timer) begin
              ok_to_end = 1;
              break;
            end else begin
              `uvm_info(LOG_NAME, $sformatf(" Resetting timer"), UVM_FULL);
              reset_timer = 0;
            end          
          end
        end        
      join_any
    endtask

I hope this at least shows some of the drawbacks of putting objections in the run phase.
and secondly in the scoreboard.
If there is a big desire to but objections in the scoreboard I think that is ok, but I think we should require objections to only go in the phase_ready_to_end function when outside of the test.

This guide lists SV interfaces as problematic and that their use is discouraged. Do you have a specific rationale for this?

My own experience is that SV Interfaces are invaluable for verification code, and can be used for RTL as long as care is taken with synthesis. I do not know if SV Interfaces are supported by all FPGA synthesis tools (its been a while since I've tried that).

It would be nice to have a "parser torture test" for our SV style guide: a synthesizable design which uses all the features we're using in our style guide. We can then use this to quickly check if tools are able to understand our SV subset. Essentially, copying all code examples into a couple files ...

I'd not go beyond a parser test, i.e. it's out of scope if the tool actually understands and processes the constructs right.

The need for this showed up in lowRISC/ibex#117. (Quartus Standard doesn't support some of the constructs we're using.)

in the style guide you wrote that FSM must be implement with two process blocks. why? why not only one? is there a reason or only for the standard?

I would like to remove the clause that restricts the usage of wildcard import of the package.

In some cases, it is quite useful not to type the package namespace every time.
The concerns causing the restriction above are the collision of the names between the module and the package and the usage of wildcard import outside of the module. But if the wildcard import is used inside the module and the same IP that defines the package, it is unlikely to have collisions.

So I would like to suggest below cases are permitted in the style guide.

1. wildcard import at the module header is permitted if the package is defined in the same IP.

module mod_a import mod_a_pkg::*; (
);

endmodule

2. wildcard import is permitted inside the module body if the package is defined in the same IP.

module mod_a ();

  import mod_a_pkg::*;

endmodule

Other than the cases above, wildcard import is not allowed. For instance, below example can create name collision in the module following mod_a in the source list.

// mod_a.sv
import mod_a_pkg::*;

module mod_a ();


// mod_b.sv
module mod_b ();
  // mod_b cannot use the name defined in mod_a_pkg
endmodule

Other bad examples:

// wildcard import for other packages outside of the IP
module mod_a import mod_b_pkg::*; ();

module mod_a ();

  // wildcard import of different packages.
  import mod_b_pkg::*;

endmodule

directory structure for above examples

hw/ip
    | mod_b/rtl/mod_b_pkg.sv
    \ mod_a/rtl/
        | mod_a.sv
        \ mod_a_pkg.sv

CC: @tjaychen @msfschaffner

This Ibex issue: lowRISC/ibex#548 has raised the question of how we should deal with Xs going into case statements where we cannot avoid that X. From the style guide a typical case statement should look something like

`ASSERT_KNOWN(CaseInputKnown, case_input, clk_i, !rst_ni)
always_comb begin
  case_output = 4'b0000;
  unique case (case_input)
    2'b00: case_output = 4'b0001;
    2'b01: case_output = 4'b0010;
    2'b10: case_output = 4'b0100;
    2'b11: case_output = 4'b1000;
    default: ;
  endcase
end

How should we deal with the case where case_input could be X? e.g. it could come direct from a top-level input that the DV environment may drive to X or from the output of a memory.

In such cases there's generally some kind of valid signal (case_valid) that lets us know that case_input can be ignored and hence case_output should be ignored.

You could say case_input must be qualified by case_valid to remove the X from the case input

logic [1:0] case_input_qualified;

assign case_input_qualified = case_input & {2{case_valid}};
`ASSERT_KNOWN(CaseInputKnown, case_input_qualified, clk_i, !rst_ni)
always_comb begin
  case_output = 4'b0000;
  unique case (case_input_qualified)
    2'b00: case_output = 4'b0001;
    2'b01: case_output = 4'b0010;
    2'b10: case_output = 4'b0100;
    2'b11: case_output = 4'b1000;
    default: ;
  endcase
end

Though this can lead to timing issues. In the case in the linked Ibex issue the valid signal (which is whether or not the instruction is illegal) is fairly late signal which we don't want to factor into the decoder outputs that lead to the X going into a case statement input.

Another option is to allow the valid signal to qualify the assertion:

`ASSERT(CaseInputKnown, case_valid |-> !$isunknown(case_input), clk_i, !rst_ni)
always_comb begin
  case_output = 4'b0000;
  unique case (case_input)
    2'b00: case_output = 4'b0001;
    2'b01: case_output = 4'b0010;
    2'b10: case_output = 4'b0100;
    2'b11: case_output = 4'b1000;
    default: ;
  endcase
end

I did also wonder if we could use assertions on the case output instead:

`ASSERT_KNOWN(CaseOutputKnown, case_output, clk_i, !rst_ni)
always_comb begin
  case_output = 4'b0000;
  unique case (case_input)
    2'b00: case_output = 4'b0001;
    2'b01: case_output = 4'b0010;
    2'b10: case_output = 4'b0100;
    2'b11: case_output = 4'b1000;
    default: ;
  endcase
end

Though given the coding style already says you should avoid explicit Xs I don't think this really gives us anything.

Really this comes down to what is the ASSERT_KNOWN really for? As we also include a default statement we don't have the simulation/synthesis mismatch issue where a simulated X gives us latching behaviour and hence want to stop that from occurring in simulation. So I see it more as a useful flag showing we've got some X's in the design that need investigating in which case qualifying the assertion with the valid signal seems reasonable.

The RTL guidelines use various phrases, punctuation, and spacing for simulation-synthesis mismatches including:

• "simulation-synthesis mismatch"
• "synthesis-simulation mismatch"
• "simulation/synthesis mismatch"
• "simulation / synthesis mismatch"
• "synthesis mismatch"

Would be ideal if one form was chosen and used consistently throughout (e.g. "simulation-synthesis mismatch" singular, "simulation-synthesis mismatches" plural). Optionally, create an appendix section on simulation-synthesis mismatches and make all mentions of simulation-synthesis mismatch a hyperlink to this section to help drive consistency and provide additional background.

Carried over from lowRISC/opentitan#901

I think it is time to revisit whether case-inside is supported by the tools, and remove that restriction from the style guide if it is, see
https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#wildcards-in-case-items

The case-inside statement can come in pretty handy for address decoding, and afaik, the debug module uses it, which also means that all our tools we've used so far are able to cope with it:
https://github.com/pulp-platform/riscv-dbg/blob/811b2d707795a5044d63a68f747b2f9cd29d3a88/src/dm_mem.sv#L207-L212

Can you please elaborate ?

Look at what can be done in the style guide to encourage low power design and prevent other potential implementation issues, as a couple of examples:

1. Flop resets/enables

To get decent power usage it's essential that flops are clocked gated and in a way that results in correct clock gate implementation. The style guide could insist all flops are written with an enable in a particular style unless there is a justification for it not to have one. As a related point it could insist flops only have resets if they actually require them

logic       data_valid_q, data_valid_d;
logic [7:0] data_q, data_d;
logic       data_en

always @(posedge clk_i or negedge rst_ni) begin
  if(~rst_ni) begin
    data_valid_q <= 1'b0;
  end else begin
    data_valid_q <= data_valid_d;
  end
end

always @(posedge clk_i) begin
  if(data_en) begin
    data_q <= data_d;
  end
end

2. Feedthroughs on IO

We may want to discourage feedthrough IO paths (input connected to output only via combinational logic) on top-level blocks unless they have been justified. The reason being these can lead to timing issues, as paths to top-level block IO may already to be long, with feedthroughs you'll join two long paths together so at the very least they need to be highlighted in documentation so users of the IP are aware of the potential issues when integrating.

How do we want to format named port lists in module instantiations in SystemVerilog source code? The style guide is currently silent on this topic, but uses in examples the "align" formatting shown below.

Verible give us the following options:

    --named_port_alignment (Format named port connections:
      {align,flush-left,preserve,infer}); default: infer;

align and flush-left are the actual choices. preserve doesn't change formatting, and infer tries to guess the formatting based on what the code used, which isn't a good choice for a style guide.

Option 1: align

Verible with --named_port_alignment=align. Note that tabular alignment is always done in "blocks" (as indicated by an empty line in between).

module my_module;

  mod u_mod (
      .clk_i,
      .rst_ni,
      .in_1 (my_signal_in),
      .out_2(my_signal_out),

      .in_another_block_i(my_signal_in),
      .in3               (something)
  );

endmodule

Option 2: flush-left

Verible with --named_port_alignment=flush-left:

module my_module;

  mod u_mod (
      .clk_i,
      .rst_ni,
      .in_1(my_signal_in),
      .out_2(my_signal_out),

      .in_another_block_i(my_signal_in),
      .in3(something)
  );

endmodule

from @NilsGraf originally

Suggestion: Use 2 space indent everywhere (including module port declarations, module parameter lists, structs and enums). Use 4 space indent only for wrapping long lines.

Rational
There seems to be a mismatch between our Verilog style guide and the referred to C++ style guide. For example, the C++ style guide uses 2 spaces for structs and enum declarations, see this example:

struct UrlTableProperties {
  string name;
  int num_entries;
  static Pool<UrlTableProperties>* pool;
};

And this example:

enum UrlTableErrors {
  kOK = 0,
  kErrorOutOfMemory,
  kErrorMalformedInput,
};

However, our style-guide uses 4 spaces for enums, see here:

typedef enum logic [1:0] {  // A 2-bit enumerated type
    AccWrite,
    AccRead,
    AccPause
} access_e;

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.

• Disallow calling of functions from assign/wire. always_comb should be used instead. By @sifferman and @marnovandermaas
• Disallow all wire assignments during declaration, (wire [7:0] test = test_func()). By @GregAC
• Disallow argumentless functions. By @nbdd0121

Full explanation by @GregAC in #66 (comment)

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)

Would you be willing to put an official position in your style guide about use of wand and wor nets?

Found in VerilogCodingStyle.md at line 168:

Use standard formatting for comments, including C-like formatting for TODO and deprecation.

Deprecation Comments don't exist any more in Google's guide.