[prim,rtl] Rewrite a case statement to avoid unreachable case #26072
Conversation
|
Note that this interacts with the coverage exclusions in #25705. If that PR lands before this one, we will need to amend this PR to drop some exclusions from the @antmarzam FYI (but I'm happy to do the leg work if it needs doing) |
No worries, since you pushed this PR I'll clean up my other PR so we can get it merged on master! |
| default: begin | ||
| state_d = StIdle; | ||
| end |
There was a problem hiding this comment.
Don't we keep these in case synthesis takes liberties with the state encoding? (e.g. converting to one-hot for some reason)
Maybe it's fine as long as there is a default, but the examples in the style guide all show "recovery" circuits.
@vogelpi @andreaskurth to help me in case my memory / understanding has gone rotten. ;)
There was a problem hiding this comment.
I think that we just need some default. The previous code was something like "If the state is silly then do X" and I propose change it to "If the state is silly, behave as if it is the following sensible value".
Clearly a bad idea if this is some sparsely encoded FSM in a security module! Here, we have exactly two states, represented by a single bit...
There was a problem hiding this comment.
Can't we just put an empty default here, i.e., default:;. IMHO this would be more readable. Does this also create a coverage issue?
There was a problem hiding this comment.
The empty default is also what we have in our style guide: https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#case-statements
There was a problem hiding this comment.
Hmm. Good question: I'm not sure. I'll try it out.
There was a problem hiding this comment.
Sadly, it doesn't solve the problem. I think the point is that this is "branch coverage of the cond statement", so the question is whether we jump to the default case (instead of whether we run every line in that case).
I did the obvious rewrite:
and here are the results:
Instead of the default case being unreachable (unless state_q is X), rewrite things so that the last item is caught by the default case. This is clearly safe at the moment (there are just two possible states!) but this adds the GoodState_A assertion to make sure no future RTL change can make this unsafe. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
prim-reg-cdc-arb-default
branch
from
3420d7f to
2817263
Compare
Instead of the default case being unreachable (unless state_q is X), rewrite things so that the last item is caught by the default case.
This is clearly safe at the moment (there are just two possible states!) but this adds the GoodState_A assertion to make sure no future RTL change can make this unsafe.