Hazard3 icon indicating copy to clipboard operation
Hazard3 copied to clipboard

Simulation race condition in frontend.v

Open chrmard opened this issue 5 months ago • 1 comments

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

chrmard avatar Sep 19 '24 11:09 chrmard