lowRISC / lowRISC/style-guides
Recommendations around xprop in simulations
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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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