lowRISC / lowRISC/style-guides

Recommendations around xprop in simulations

Open
#31 11 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
No language data
Stars
541
Forks
132
PR merge metrics
No merged PRs in 30d

Description

The style guide currently recommends the following style for case statements

```systemverilog
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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the style guide's recommended SystemVerilog case-statement pattern and the linked Ibex decoder and ASSERT_KNOWN example. Compare the xprop behavior with the three proposed approaches, then document and agree on one clear recommendation, including any required assertion guidance or macro changes.

Written by the indexing model from the issue text.

Assessment

Domain
documentation
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.