Wren6991 / Hazard3

3-stage RV32IMACZb* processor with debug
Apache License 2.0
696 stars 47 forks source link

Simulation race condition in frontend.v #21

Open chrmard opened 1 month ago

chrmard commented 1 month ago

For the fifo definition hazard3_frontend.v uses the same signals in a synchronous process (e.g. in fifo_update for fifo_mem[0..FIFO_DEPTH-1]) and an asynchronous process (e.g. boundary_conditions for fifo_mem[FIFO_DEPTH]).

From verilog language definition this is correct, and most simulator and synthesis have no issue with this. I checked the code on some Verilator versions, they have a problem (for example Verilator 5.018 2023-10-30 rev v5.018). Cadence and Synopsys tools have not.

Observation on Verilator: The fifo_valid and fifo_valid_m1 logic is not simulated over delta cycles, it is updated only once when the fifo_mem signal is processed.

To overcome this situation, the process can be split. It has no functional impact; it separates only the "shared async/sync signals" from the pure async signals. With this patch the Verilator problem is solved. Apply this patch would prevent other user to debug this tool issue again.

diff --git a/hdl/hazard3_frontend.v b/hdl/hazard3_frontend.v
index ac30c77..4f55585 100644
--- a/hdl/hazard3_frontend.v
+++ b/hdl/hazard3_frontend.v
@@ -131,11 +131,14 @@ wire              fifo_pop;
 wire              fifo_dbg_inject = DEBUG_SUPPORT && dbg_instr_data_vld && dbg_instr_data_rdy;

 always @ (*) begin: boundary_conditions
-       integer i;
        fifo_mem[FIFO_DEPTH] = mem_data;
        fifo_predbranch[FIFO_DEPTH] = 2'b00;
        fifo_err[FIFO_DEPTH] = 1'b0;
        fifo_valid_hw[FIFO_DEPTH] = 2'b00;
+end
+
+always @ (*) begin: fifo_valdidation
+       integer i;
        for (i = 0; i < FIFO_DEPTH; i = i + 1) begin
                fifo_valid[i] = |EXTENSION_C ? |fifo_valid_hw[i] : fifo_valid_hw[i][0];
                // valid-to-right condition: i == 0 || fifo_valid[i - 1], but without

There is one other linting problem in the same file. As verilog interpretes undefined signals as false in && condition, the behaviour is not impacted. The other usages of this signal are gated by BRANCH_PREDICTOR define itself.

@@ -224,6 +227,7 @@ if (BRANCH_PREDICTOR) begin: have_btb
 end else begin: no_btb
        always @ (*) begin
                btb_src_addr = {W_ADDR{1'b0}};
+               btb_src_size = 1'b0;
                btb_target_addr = {W_ADDR{1'b0}};
                btb_valid = 1'b0;
        end