fix default assignments #617
Conversation
| @@ -1015,6 +1015,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*; | |||
| csr_wdata_int = csr_wdata; | |||
| csr_we_int = 1'b0; | |||
| end | |||
| default:; | |||
There was a problem hiding this comment.
This case statement is already complete. I believe we would get a Lint warning when adding a default here.
| @@ -307,6 +307,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*; | |||
| unique case (imm_a_mux_sel) | |||
| IMMA_Z: imm_a = imm_z_type; | |||
| IMMA_ZERO: imm_a = '0; | |||
| default: imm_a = '0; | |||
There was a problem hiding this comment.
This case statement is already complete. I believe we would get a Lint warning when adding a default here.
There was a problem hiding this comment.
However, if the above is true, then we should have a warning for jalr_fw_mux.
|
@silabs-oysteink Can you check with our Lint setup if this PR would increase the number of default related warnings? Some of the fixes in this PR are definitely good, but maybe not all. |
Silabs-ArjanB
added
Component:RTL
|
Hi @Silabs-ArjanB , indeed I got most of the time just warnings, even when the case was somehow complete (with Verilator) |
|
@davideschiavone My point is that I believe that we sometimes get warnings because of adding a default (but we will recheck that). |
|
ah no I don't think so (for my case I mean), because using the branch of the PR is shutting down those warnings, again, with Verilator. Other tools may behave differenlty |
|
I think the changes are good. They do lead to an increased amount of lint warnings though, both for the fully covered cases with added default, and for those that are not full which now gets a 'reachable default'. I think it is better to be consistent with defaults, and instead waive warnings in lint if wanted. |
silabs-oysteink
removed
the
Status:Do-not-merge
No description provided.