From cd56694fb00a38fb057dd38255c5452b78cc40f9 Mon Sep 17 00:00:00 2001 From: Florian Zaruba Date: Fri, 28 Apr 2017 15:56:44 +0200 Subject: [PATCH] :bug: Various bugfixes in store_queue --- Makefile | 2 +- store_queue.sv | 79 ++++++++-------- tb/agents/store_queue_if/store_queue_if.sv | 4 +- tb/store_queue_tb.sv | 105 ++++++++++++++++++--- tb/wave_store_queue.do | 44 +++++++++ 5 files changed, 176 insertions(+), 58 deletions(-) create mode 100644 tb/wave_store_queue.do diff --git a/Makefile b/Makefile index fb1afa11e..91e7745ec 100644 --- a/Makefile +++ b/Makefile @@ -57,7 +57,7 @@ $(tests): # Optimize top level vopt${questa_version} ${compile_flag} $@_tb -o $@_tb_optimized +acc -check_synthesis # vsim${questa_version} $@_tb_optimized - vsim${questa_version} -c +UVM_TESTNAME=$@_test -coverage -do "coverage save -onexit $@.ucdb; run -a; quit -code [coverage attribute -name TESTSTATUS -concise]" $@_tb_optimized + # vsim${questa_version} -c +UVM_TESTNAME=$@_test -coverage -do "coverage save -onexit $@.ucdb; run -a; quit -code [coverage attribute -name TESTSTATUS -concise]" $@_tb_optimized # User Verilator to lint the target lint: verilator ${src} --lint-only \ diff --git a/store_queue.sv b/store_queue.sv index ea67066ce..f1b110337 100644 --- a/store_queue.sv +++ b/store_queue.sv @@ -57,8 +57,6 @@ module store_queue ( // In the current implementation this is represented by a single entry and // differentiated by the is_speculative flag. - enum logic { IDLE, WAIT_RVALID } CS, NS; - struct packed { logic [63:0] address; logic [63:0] data; @@ -68,10 +66,10 @@ module store_queue ( } commit_queue_n, commit_queue_q; // we can directly output the commit entry since we just have one element in this "queue" - assign paddr_o = commit_queue_q.address; - assign data_o = commit_queue_q.data; - assign be_o = commit_queue_q.be; - assign valid_o = commit_queue_q.valid; + assign paddr_o = commit_queue_q.address; + assign data_o = commit_queue_q.data; + assign be_o = commit_queue_q.be; + assign valid_o = commit_queue_q.valid; // those signals can directly be output to the memory if assign address_o = commit_queue_q.address; @@ -81,44 +79,32 @@ module store_queue ( // memory interface always_comb begin : store_if // if there is no commit pending and the uncommitted queue is empty as well we can accept the request - automatic logic ready = ~commit_queue_q.valid; - ready_o = ready; + // if we got a grant this implies that the value was not speculative anymore and that we + // do not need to save the values anymore since the memory already processed them + automatic logic ready = ~commit_queue_q.valid | data_gnt_i; + ready_o = ready; commit_queue_n = commit_queue_q; data_we_o = 1'b1; // we will always write in the store queue data_req_o = 1'b0; - NS = CS; + // there should be no commit when we are flushing if (~flush_i) begin - case (CS) - // we can accept a new request if we were idle - IDLE: begin - if (commit_queue_q.valid) begin - data_req_o = 1'b1; - if (data_gnt_i) begin// advance to the next state if we received the grant - NS = WAIT_RVALID; - - end - end + // if the entry in the commit queue is valid and not speculative anymore + // we can issue this instruction + // we can issue it as soon as the commit_i goes high or any number of cycles later + // by looking at the is_speculative flag + if (commit_queue_q.valid & (~commit_queue_q.is_speculative | commit_i)) begin + data_req_o = 1'b1; + if (data_gnt_i) begin// advance to the next state if we received the grant + // NS = WAIT_RVALID; + // we can evict it from the commit buffer + commit_queue_n.valid = 1'b0; end - - WAIT_RVALID: begin - - if (data_rvalid_i) begin - // if we got the rvalid we can already perform a new store request - if (commit_queue_q.valid) begin - // we can evict it from the commit buffer - commit_queue_n.valid = 1'b0; - data_req_o = 1'b1; - end else // if do not need to perform another - NS = IDLE; - end else // wait here for the rvalid - NS = WAIT_RVALID; - end - - default:; - endcase + end + // we ignore the rvalid signal for now as we assume that the store + // happened end // shift the store request from the speculative buffer @@ -129,7 +115,7 @@ module store_queue ( // LSU interface // we are ready to accept a new entry and the input data is valid - if (ready & valid_i & ~commit_i) begin + if (ready & valid_i) begin commit_queue_n.address = paddr_i; commit_queue_n.data = data_i; commit_queue_n.be = be_i; @@ -137,19 +123,28 @@ module store_queue ( commit_queue_n.is_speculative = 1'b1; end + // when we flush evict the speculative store + if (flush_i & commit_queue_q.is_speculative) begin + commit_queue_n.valid = 1'b0; + end + end // registers always_ff @(posedge clk_i or negedge rst_ni) begin : proc_ if(~rst_ni) begin commit_queue_q <= '{default: 0}; - CS <= IDLE; - end else if (flush_i) begin // just empty the speculative queue - commit_queue_q <= commit_queue_n; - CS <= NS; end else begin commit_queue_q <= commit_queue_n; - CS <= NS; end end + `ifndef SYNTHESIS + `ifndef verilator + // assert that commit is never set when we are flushing this would be counter intuitive + // as flush and commit is decided in the same stage + assert property ( + @(posedge clk_i) rst_ni & flush_i |-> ~commit_i) + else $error ("You are trying to commit and flush in the same cycle"); + `endif + `endif endmodule \ No newline at end of file diff --git a/tb/agents/store_queue_if/store_queue_if.sv b/tb/agents/store_queue_if/store_queue_if.sv index 92ceb2745..8b5d0cc8d 100755 --- a/tb/agents/store_queue_if/store_queue_if.sv +++ b/tb/agents/store_queue_if/store_queue_if.sv @@ -41,8 +41,8 @@ interface store_queue_if wire [DATA_WIDTH/8-1:0] store_be; clocking mck @(posedge clk); - output flush, commit, valid, store_paddr, store_data, store_be; - input check_paddr, check_data, check_be, ready, store_valid; + output flush, commit, valid, store_paddr, store_data, store_be, store_valid; + input check_paddr, check_data, check_be, ready; endclocking diff --git a/tb/store_queue_tb.sv b/tb/store_queue_tb.sv index 7c5bdfdf0..2dab24bc9 100755 --- a/tb/store_queue_tb.sv +++ b/tb/store_queue_tb.sv @@ -20,25 +20,27 @@ module store_queue_tb; logic rst_ni, clk; + logic store_valid; + logic slave_data_gnt; mem_if slave(clk); store_queue_if store_queue(clk); store_queue dut ( - .clk_i ( clk ), - .rst_ni ( rst_ni ), - .flush_i ( store_queue.flush ), - .paddr_o ( store_queue.check_paddr ), - .data_o ( store_queue.check_data ), - .valid_o ( store_queue.valid ), - .be_o ( store_queue.check_be ), - .commit_i ( store_queue.commit ), - .ready_o ( store_queue.ready ), - .valid_i ( store_queue.store_valid ), - .paddr_i ( store_queue.store_paddr ), - .data_i ( store_queue.store_data ), - .be_i ( store_queue.store_be ), + .clk_i ( clk ), + .rst_ni ( rst_ni ), + .flush_i ( store_queue.flush ), + .paddr_o ( store_queue.check_paddr ), + .data_o ( store_queue.check_data ), + .valid_o ( store_queue.valid ), + .be_o ( store_queue.check_be ), + .commit_i ( store_queue.commit ), + .ready_o ( store_queue.ready ), + .valid_i ( store_queue.store_valid ), + .paddr_i ( store_queue.store_paddr ), + .data_i ( store_queue.store_data ), + .be_i ( store_queue.store_be ), .address_o ( slave.address ), .data_wdata_o ( slave.data_wdata ), @@ -60,6 +62,11 @@ module store_queue_tb; #10ns clk = ~clk; end + // combinatorial signals + assign store_queue.flush = 1'b0; + assign store_queue.store_valid = store_valid & store_queue.ready; + assign slave.data_gnt = slave_data_gnt; + // simulator stopper, this is suboptimal better go for coverage initial begin #10000000ns @@ -67,13 +74,85 @@ module store_queue_tb; end program testbench (mem_if slave, store_queue_if store_queue); + // data to store + class store_data; + rand logic [63:0] data; + rand logic [7:0] be; + endclass : store_data + + store_data data = new(); + semaphore sem = new(1); // ---------- // Driver // ---------- + + // make a new store if it is possible + initial begin + // reset assignment + store_valid <= 'b0; + store_queue.mck.store_paddr <= 'b0; + store_queue.mck.store_data <= 'b0; + store_queue.mck.store_be <= 'b0; + store_queue.mck.commit <= 1'b0; + wait(rst_ni); + + forever begin + @(store_queue.mck); + store_queue.mck.commit <= 1'b0; + if (store_queue.mck.ready) begin + store_queue.mck.store_paddr <= 64'h1; + store_valid <= 1'b1; + // get some randomized data + void'(data.randomize()); + store_queue.mck.store_data <= data.data; + store_queue.mck.store_be <= data.be; + + // commit a couple of cycles later + fork + commit_block: begin + sem.get(1); + @(store_queue.mck); + @(store_queue.mck); + store_queue.mck.commit <= 1'b1; + sem.put(1); + end + join_none + + end else begin + store_valid <= 1'b0; + end + end + + end + // commit part initial begin end + // grant process + initial begin + + forever begin + slave_data_gnt = 1'b0; + wait (slave.data_req); + // randomize grant delay + // repeat ($urandom_range(0,4)) @(slave.mck); + slave_data_gnt = 1'b1; + wait (~slave.data_req); + end + end + + // write memory interface + initial begin + forever begin + @(slave.mck); + if (slave.mck.data_req) + slave.mck.data_rvalid <= 1'b1; + else + slave.mck.data_rvalid <= 1'b0; + end + end + // ------------------- // Monitor && Checker diff --git a/tb/wave_store_queue.do b/tb/wave_store_queue.do new file mode 100644 index 000000000..b459477d6 --- /dev/null +++ b/tb/wave_store_queue.do @@ -0,0 +1,44 @@ +onerror {resume} +quietly WaveActivateNextPane {} 0 +add wave -noupdate /store_queue_tb/dut/clk_i +add wave -noupdate /store_queue_tb/dut/rst_ni +add wave -noupdate /store_queue_tb/dut/flush_i +add wave -noupdate /store_queue_tb/dut/paddr_o +add wave -noupdate /store_queue_tb/dut/data_o +add wave -noupdate /store_queue_tb/dut/valid_o +add wave -noupdate /store_queue_tb/dut/be_o +add wave -noupdate /store_queue_tb/dut/commit_i +add wave -noupdate /store_queue_tb/dut/ready_o +add wave -noupdate /store_queue_tb/dut/valid_i +add wave -noupdate /store_queue_tb/dut/paddr_i +add wave -noupdate /store_queue_tb/dut/data_i +add wave -noupdate /store_queue_tb/dut/be_i +add wave -noupdate /store_queue_tb/dut/address_o +add wave -noupdate /store_queue_tb/dut/data_wdata_o +add wave -noupdate /store_queue_tb/dut/data_req_o +add wave -noupdate /store_queue_tb/dut/data_we_o +add wave -noupdate /store_queue_tb/dut/data_be_o +add wave -noupdate /store_queue_tb/dut/data_gnt_i +add wave -noupdate /store_queue_tb/dut/data_rvalid_i +add wave -noupdate /store_queue_tb/dut/CS +add wave -noupdate /store_queue_tb/dut/NS +add wave -noupdate /store_queue_tb/dut/commit_queue_n +add wave -noupdate /store_queue_tb/dut/commit_queue_q +TreeUpdate [SetDefaultTree] +WaveRestoreCursors {{Cursor 1} {0 ns} 0} +quietly wave cursor active 0 +configure wave -namecolwidth 150 +configure wave -valuecolwidth 100 +configure wave -justifyvalue left +configure wave -signalnamewidth 1 +configure wave -snapdistance 10 +configure wave -datasetprefix 0 +configure wave -rowmargin 4 +configure wave -childrowmargin 2 +configure wave -gridoffset 0 +configure wave -gridperiod 1 +configure wave -griddelta 40 +configure wave -timeline 0 +configure wave -timelineunits ns +update +WaveRestoreZoom {0 ns} {13479 ns}