Further Verilator lint fixes

[marnovandermaas]

Found these issues while compiling CHERIoT Ibex with Verilator. It mainly fixes width mismatches and unused variables.

This PR does not remove all unused variables, there is one remaining width issue and two unoptimized flat issues.

[kliuMsft]

This is a lot of changes - and I am not super sure the behavior stays the same (e.g. line 347). Generally, could we do a LEC before/after such chnages? If you have already done that please do let me know. Thanks,

[marnovandermaas]

This is a lot of changes - and I am not super sure the behavior stays the same (e.g. line 347). Generally, could we do a LEC before/after such chnages? If you have already done that please do let me know. Thanks,

I'm used to making these changes through code review. I'll mark this PR as draft until I get a chance to resolve the conflicts and look at equivalence checking.

[kliuMsft]

Thanks, I appreciate it. And I agree normally it would be done early in the design phase after code review, etc. However we are pretty late in the game already in this case and a little extra caution is probably a good thing.

[marnovandermaas]

Ok, I ran JasperGold's Sequential Equivalence Checker on the ibexc_top module before and after the fixes. It says that from an interface point of view they are the same. The only differences it detected were output pins that are undriven before the changes but connected by my fixes: alert_major_bus_o, alert_major_internal_o, alert_minor_o and scramble_req_o. I don't have access to any other equivalence checking tools unfortunately.

This is the output of the check:

   Interface Check Summary
=============================
primary_input       - PASSED
primary_output      - PASSED
user_bbox_input     - PASSED
user_bbox_output    - PASSED
auto_bbox_input     - PASSED
auto_bbox_output    - PASSED
user_task_stopat    - PASSED
user_env_stopat     - PASSED
setup_stopat        - PASSED
internal_undriven   - PASSED
x_assignment        - PASSED
x_default_assignment- PASSED
x_index_out_of_range- PASSED
x_divide_by_zero    - PASSED
x_misc_undriven     - PASSED
x_low_power         - PASSED
x_bus_contention    - PASSED
x_bus_floating      - PASSED
reset_x             - PASSED
imp_assumptions     - PASSED
=============================
Overall interface check result - PASSED

[kliuMsft]

That's excellent, thanks. Will approve and merge then.

[kliuMsft]

I noticed the PR is still in draft status - any remaining conflict?

[marnovandermaas]

I noticed the PR is still in draft status - any remaining conflict?

No conflicts as far as I'm aware, just forgot to mark as ready to review yesterday.

[kliuMsft]

About 'int typecasting (range_ok in cheri_trvk_stage) and cheri_regfile - why are we casting those to int (which I think is signed by default)? The intention in both places is to do unsigned math. It may not matter here is the data width is less than 32-bit however it seems unnecessary confusion to me.

[marnovandermaas]

About 'int typecasting (range_ok in cheri_trvk_stage) and cheri_regfile - why are we casting those to int (which I think is signed by default)? The intention in both places is to do unsigned math. It may not matter here is the data width is less than 32-bit however it seems unnecessary confusion to me.

Yes, you are right. SystemVerilog makes it a bit difficult to do casting to compound types like int unsigned. I've pushed an example solution:

diff --git a/rtl/cheri_pkg.sv b/rtl/cheri_pkg.sv
index 8ca19314..0ef0c0d3 100644
--- a/rtl/cheri_pkg.sv
+++ b/rtl/cheri_pkg.sv
@@ -42,6 +42,9 @@ package cheri_pkg;
   parameter logic [2:0] OTYPE_SENTRY     = 3'd1;
   parameter logic [2:0] OTYPE_UNSEALED   = 3'd0;
 
+  // For ease of casting
+  typedef int unsigned u_int_t;
+
   // Compressed (regFile) capability type
   typedef struct packed {
     logic                valid;
diff --git a/rtl/cheri_regfile.sv b/rtl/cheri_regfile.sv
index 5ab1d81d..199e3e5c 100644
--- a/rtl/cheri_regfile.sv
+++ b/rtl/cheri_regfile.sv
@@ -142,8 +142,8 @@ module cheri_regfile import cheri_pkg::*; #(
     assign rf_cap[i] = rf_cap_q[i];
   end
 
-  assign rcap_a = (int'(raddr_a_i) < NCAPS) ? rf_cap[raddr_a_i] : NULL_REG_CAP;
-  assign rcap_b = (int'(raddr_b_i) < NCAPS) ? rf_cap[raddr_b_i] : NULL_REG_CAP;
+  assign rcap_a = (u_int_t'(raddr_a_i) < NCAPS) ? rf_cap[raddr_a_i] : NULL_REG_CAP;
+  assign rcap_b = (u_int_t'(raddr_b_i) < NCAPS) ? rf_cap[raddr_b_i] : NULL_REG_CAP;
 
   if (CheriPPLBC) begin : g_regrdy
 
diff --git a/rtl/cheri_trvk_stage.sv b/rtl/cheri_trvk_stage.sv
index 78cf24dd..0025f549 100644
--- a/rtl/cheri_trvk_stage.sv
+++ b/rtl/cheri_trvk_stage.sv
@@ -61,7 +61,7 @@ module cheri_trvk_stage #(
   assign tsmap_addr_o  = tsmap_ptr[15:5];
 
   // not a sealling cap and pointing to valid TSMAP range
-  assign range_ok      = (int'(tsmap_ptr[31:5]) <= TSMapSize) &&
+  assign range_ok      = (u_int_t'(tsmap_ptr[31:5]) <= TSMapSize) &&
                          ~((in_cap_q.cperms[4:3]==2'b00) && (|in_cap_q.cperms[2:0]));
   assign tsmap_cs_o    = (cpu_op_valid_q[0] | tbre_op_valid_q[0]) & cap_good_q[0];

What do you think? It does add an extra typedef to cheri_pkg.

[kliuMsft]

That should work. However, why do we need typecasting in those cases? Does it help resolve any ambiguities (I can't quite see it) or there is some kind of blanket lint policy regarding arithmetics? If not I'd rather have less code..

[marnovandermaas]

That should work. However, why do we need typecasting in those cases? Does it help resolve any ambiguities (I can't quite see it) or there is some kind of blanket lint policy regarding arithmetics? If not I'd rather have less code..

I'm not sure if it resolves any ambiguities but it stops Verilator from complaining that we are comparing two values of different bit width. Unsigned integers are 32 bits while raddr_x_i and tsmap_ptr are less.

[marnovandermaas]

I force pushed to resolve the conflicts in rtl/ibex_controller.sv.

[kliuMsft]

Hopefully not trying to be obnoxious here.. But I am disagreeing with enforcing this linting rule. verilog/sv is fairly clear about the behavior in this case (0-expand to the widest operand first then perform comparison). As such, the change is unnecessary and would make the code more complex and less readable.

[kliuMsft]

@marnovandermaas, also if we really want to match width explicitly, wouldn't something like 32'({tsmap_ptr[31:5]}) work? That way at least we stay unsigned and don't have to define a new type..

[marnovandermaas]

Hopefully not trying to be obnoxious here.. But I am disagreeing with enforcing this linting rule. verilog/sv is fairly clear about the behavior in this case (0-expand to the widest operand first then perform comparison). As such, the change is unnecessary and would make the code more complex and less readable.

Ok, I've removed these changes. I can always add some lint waivers for these particular cases.

[kliuMsft]

Thanks - should be merged into main now.