Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed todo's related to mul_en/div_en i EX stage #902

Merged
merged 7 commits into from
Aug 25, 2023
Merged

Fixed todo's related to mul_en/div_en i EX stage #902

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
10115bd
Removed halt_ex and kill_ex from mul_en_gated and div_en_gated in the…
silabs-oysteink Aug 8, 2023
d08568d
Updates after review.
silabs-oysteink Aug 14, 2023
3a9f78b
Comment update.
silabs-oysteink Aug 14, 2023
4287cc3
Update cv32e40x_ex_stage.sv
Silabs-ArjanB Aug 15, 2023
14e25f7
Revert to original case statement and factoring in !valid/kill/halt a…
silabs-oysteink Aug 17, 2023
6e0214e
Merge branch 'silabs-oysteink-div-mul-todo' of github.com:silabs-oyst…
silabs-oysteink Aug 17, 2023
ec7f6e7
Review fixes.
silabs-oysteink Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions rtl/cv32e40x_div.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ module cv32e40x_div import cv32e40x_pkg::*;
output logic [5:0] alu_shift_amt_o,
input logic [31:0] alu_op_b_shifted_i,

// Divider enable
input logic div_en_i,
// Controller inputs
input logic halt_i,
input logic kill_i,

// Input handshake
input logic valid_i,
Expand Down Expand Up @@ -141,12 +142,12 @@ module cv32e40x_div import cv32e40x_pkg::*;
end
endgenerate

assign alu_clz_en_o = div_en_i;
assign alu_clz_en_o = valid_i;

// Deternmine initial shift of divisor
assign op_b_is_neg = op_b_i[31] & div_signed;
assign alu_shift_amt_o = alu_clz_result_i ;
assign alu_shift_en_o = div_en_i;
assign alu_shift_amt_o = alu_clz_result_i;
assign alu_shift_en_o = valid_i;

// Check for op_b_i == 0
assign op_b_is_zero = !(|op_b_i);
Expand Down Expand Up @@ -258,9 +259,9 @@ module cv32e40x_div import cv32e40x_pkg::*;
end

DIV_FINISH: begin
valid_o = 1'b1;
valid_o = valid_i && !(halt_i || kill_i); // No valid outputs while halted or killed
ready_o = (ready_i && !halt_i) || kill_i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now halt impacts ready_o but not next_state. I see that next-state is not used later on in that case, but it is just difficult to understand like this.

Maybe keep case statement as in original design and assume valid_i=1, halt_i=0, kill_i=0 within the case and deal with exceptions to that assumption after the case statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to original case statement.

if (ready_i) begin
ready_o = 1'b1;
next_state = DIV_IDLE;
end
end
Expand All @@ -270,7 +271,7 @@ module cv32e40x_div import cv32e40x_pkg::*;
endcase

// Allow kill at any time
if (!valid_i) begin
if (!valid_i || kill_i) begin
next_state = DIV_IDLE;
ready_o = 1'b1;
valid_o = 1'b0;
Expand Down Expand Up @@ -304,6 +305,8 @@ module cv32e40x_div import cv32e40x_pkg::*;
comp_inv_q <= 1'b0;
res_inv_q <= 1'b0;
end else begin
// If stage is halted, the divider should have no state updates
if (!halt_i || kill_i) begin
state <= next_state;
remainder_q <= remainder_d;
divisor_q <= divisor_d;
Expand All @@ -312,6 +315,7 @@ module cv32e40x_div import cv32e40x_pkg::*;
div_rem_q <= div_rem_d;
comp_inv_q <= comp_inv_d;
res_inv_q <= res_inv_d;
end
end
end

Expand Down
30 changes: 17 additions & 13 deletions rtl/cv32e40x_ex_stage.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,6 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
logic [31:0] div_result;

// Gated enable signals factoring in instr_valid)
logic mul_en_gated;
logic div_en_gated;
logic lsu_en_gated;

// Divider signals
Expand All @@ -116,6 +114,9 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
logic [5:0] div_shift_amt;
logic [31:0] div_op_b_shifted;

// Multiplier signals
logic mul_en;

// Misc signals
logic previous_exception;
logic first_op;
Expand All @@ -126,14 +127,15 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;

assign instr_valid = id_ex_pipe_i.instr_valid && !ctrl_fsm_i.kill_ex && !ctrl_fsm_i.halt_ex;

// todo: consider not factoring halt_ex into the mul/div/lsu_en_gated below
// Halting EX currently reset state of these units. The IF stage sequencer is _not_ reset on a halt_if.
// Maybe we need to split out valid and halt into the submodules?
assign mul_en_gated = id_ex_pipe_i.mul_en && instr_valid; // Factoring in instr_valid to kill mul instructions on kill/halt
assign div_en_gated = id_ex_pipe_i.div_en && instr_valid; // Factoring in instr_valid to kill div instructions on kill/halt
// The multiplier and divider both factor in halt_ex and kill_ex.
// MUL/DIV instructions in flight will keep state while halted, and reset state on kill.
assign mul_en = id_ex_pipe_i.mul_en && id_ex_pipe_i.instr_valid; // Valid MUL in EX, not affected by kill/halt
assign div_en = id_ex_pipe_i.div_en && id_ex_pipe_i.instr_valid; // Valid DIV in EX, not affected by kill/halt

// The lsu_en_gated factors in halt_ex and kill_ex via instr_valid.
// A halted or killed LSU instruction must not generate a data_req to ensure no OBI protocol is violated.
assign lsu_en_gated = id_ex_pipe_i.lsu_en && instr_valid; // Factoring in instr_valid to suppress bus transactions on kill/halt

assign div_en = id_ex_pipe_i.div_en && id_ex_pipe_i.instr_valid; // Valid DIV in EX, not affected by kill/halt

// If pipeline is handling a valid CSR AND the same instruction is accepted by the eXtension interface
// we need to convert the instruction to an illegal instruction and signal commit_kill to the eXtension interface.
Expand Down Expand Up @@ -250,11 +252,10 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
// Result
.result_o ( div_result ),

// divider enable, not affected by kill/halt
.div_en_i ( div_en ),

// Handshakes
.valid_i ( div_en_gated ),
.halt_i ( ctrl_fsm_i.halt_ex ),
.kill_i ( ctrl_fsm_i.kill_ex ),
.valid_i ( div_en ),
.ready_o ( div_ready ),
.valid_o ( div_valid ),
.ready_i ( wb_ready_i )
Expand Down Expand Up @@ -300,8 +301,11 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
// Result
.result_o ( mul_result ),

.halt_i ( ctrl_fsm_i.halt_ex ),
.kill_i ( ctrl_fsm_i.kill_ex ),

// Handshakes
.valid_i ( mul_en_gated ),
.valid_i ( mul_en ),
.ready_o ( mul_ready ),
.valid_o ( mul_valid ),
.ready_i ( wb_ready_i )
Expand Down
24 changes: 13 additions & 11 deletions rtl/cv32e40x_mult.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ module cv32e40x_mult import cv32e40x_pkg::*;

output logic [31:0] result_o,

input logic halt_i,
input logic kill_i,

output logic ready_o,
output logic valid_o,
input logic ready_i
Expand Down Expand Up @@ -115,7 +118,7 @@ module cv32e40x_mult import cv32e40x_pkg::*;
mulh_acc_next = mulh_acc;

// Case statement assumes valid_i = 1; the valid_i = 0 scenario
// is handled after the case statement.
// is handled after the case statement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment (no longer accurate)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment seems still there

case (mulh_state)
MUL_ALBL: begin
if (operator_i == MUL_H) begin
Expand All @@ -126,11 +129,8 @@ module cv32e40x_mult import cv32e40x_pkg::*;
end
else begin
// Single cycle multiplication
valid_o = 1'b1;

if (ready_i) begin
ready_o = 1'b1;
end
valid_o = valid_i && !(halt_i || kill_i);
ready_o = (ready_i && !halt_i) || kill_i;
end
end

Expand All @@ -150,12 +150,12 @@ module cv32e40x_mult import cv32e40x_pkg::*;
end

MUL_AHBH: begin
valid_o = 1'b1;
valid_o = valid_i && !(halt_i || kill_i);
mulh_a = mulh_ah;
mulh_b = mulh_bh;
ready_o = (ready_i && !halt_i) || kill_i;

if (ready_i) begin
ready_o = 1'b1;
mulh_state_next = MUL_ALBL;
mulh_acc_next = '0;
end
Expand All @@ -164,7 +164,7 @@ module cv32e40x_mult import cv32e40x_pkg::*;
endcase

// Allow kill at any time
if (!valid_i) begin
if (!valid_i || kill_i) begin
mulh_state_next = MUL_ALBL;
ready_o = 1'b1;
valid_o = 1'b0;
Expand All @@ -177,8 +177,10 @@ module cv32e40x_mult import cv32e40x_pkg::*;
mulh_acc <= '0;
mulh_state <= MUL_ALBL;
end else begin
mulh_acc <= mulh_acc_next;
mulh_state <= mulh_state_next;
if (!halt_i || kill_i) begin
mulh_acc <= mulh_acc_next;
mulh_state <= mulh_state_next;
end
end
end

Expand Down
13 changes: 9 additions & 4 deletions sva/cv32e40x_mult_sva.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ module cv32e40x_mult_sva
input logic [16:0] mulh_bl,
input logic [16:0] mulh_ah,
input logic [16:0] mulh_bh,
input logic [33:0] int_result);
input logic [33:0] int_result,
input logic halt_i,
input logic kill_i);

////////////////////////////////////////
//// Assertions on module boundary ////
Expand All @@ -54,13 +56,16 @@ module cv32e40x_mult_sva
assert property (@(posedge clk) disable iff (!rst_n)
(valid_i && (operator_i == MUL_M32)) |-> (result_o == mul_result))
else `uvm_error("mult", "MUL result check failed")
a_mul_valid : // check that MUL result is immediately qualified by valid_o
a_mul_valid : // check that MUL result is immediately qualified by valid_o unless EX is halted or killed
assert property (@(posedge clk) disable iff (!rst_n)
(valid_i && (operator_i == MUL_M32)) |-> valid_o)
(valid_i && (operator_i == MUL_M32) &&
!(halt_i || kill_i))
|->
valid_o)
else `uvm_error("mult", "MUL result wasn't immediately valid")


// Check result for all MULH flavors
// Check result for all MULH flavors
logic mulh_result_valid;
assign mulh_result_valid = ((operator_i == MUL_H) && valid_i) && valid_o;

Expand Down