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.

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

Added a 'first_op' to track the first operation of multi operation in… #605

Merged

Conversation

silabs-oysteink
Copy link
Contributor

…structions through the pipeline.

RVFI will now only latch rs1/2_rdata when the first operation of a multi operation instruction is in the ID stage.

Added assertions to check that jump targets are stable when remaining in ID for multiple cycles.
Added assertion to check that RVFI may use operand_a_fw also for JALR instructions, although JALR uses a different bypass mux in RTL.

SEC clean.

Signed-off-by: Oystein Knauserud [email protected]

…structions through the pipeline.

RVFI will now only latch rs1/2_rdata when the first operation of a multi operation instruction is in the ID stage.

Added assertions to check that jump targets are stable when remaining in ID for multiple cycles.
Added assertion to check that RVFI may use operand_a_fw also for JALR instructions, although JALR uses a different bypass mux in RTL.

SEC clean.

Signed-off-by: Oystein Knauserud <[email protected]>
bhv/cv32e40x_rvfi.sv Show resolved Hide resolved
@@ -256,7 +256,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
// Blocking on branch_taken_q, as a jump has already been taken
assign jump_taken_id = jump_in_id && !branch_taken_q;

// todo: RVFI does not use jump_taken_id (which is not in itself an issue); we should have an assertion showing that the target address remains constant during jump_in_id; same remark for branches
// Note: RVFI does not use jump_taken_id (which is not in itself an issue); An assertion in id_stage_sva checks that the jump target remains stable;
// todo: Do we need a similar stability check for branches?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes; just add it now (will be less work than picking it up a month from now)

@@ -238,6 +238,9 @@ module cv32e40x_core import cv32e40x_pkg::*;
logic lsu_valid_wb;
logic lsu_ready_1;

logic lsu_first_op_ex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move signals to between line 226 and 227

@@ -123,7 +123,8 @@ module cv32e40x_if_stage import cv32e40x_pkg::*;
// eXtension interface signals
logic [X_ID_WIDTH-1:0] xif_id;

// Flag for last operation - used by Zc*
// Flag for first and last operation - used by Zc*
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove used by Zc* comment (it is more general (I hope at least))

rtl/cv32e40x_if_stage.sv Show resolved Hide resolved
assign seq_instr = '0;
assign seq_ready = 1'b1;
assign seq_first = 1'b1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably it won't matter much, but I think that seq_first = 1'b0 is a more logical default if there is no sequencer

@@ -52,6 +52,8 @@ module cv32e40x_load_store_unit import cv32e40x_pkg::*;

// Stage 0 outputs (EX)
output logic lsu_split_0_o, // Misaligned access is split in two transactions (to controller)
output logic lsu_first_op_0_o, // First operation is active in EX
Copy link
Contributor

Choose a reason for hiding this comment

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

Align comments

@@ -387,6 +389,9 @@ module cv32e40x_load_store_unit import cv32e40x_pkg::*;
end
end

assign lsu_last_op_0_o = !lsu_split_0_o;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment that these signals are only valid when lsu_en = 1 (I think)

@@ -125,5 +135,27 @@ module cv32e40x_id_stage_sva
$onehot0({alu_en, div_en, mul_en, csr_en, sys_en, lsu_en, xif_en}))
else `uvm_error("id_stage", "Multiple functional units enabled")

// Assert that jalr_fw has the same value as operand_a_fw when a jump is taken
// Only checking for JALR, as regular JAL do not use any RF or bypass operands for the jump target.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add reason why this needs to be checked (i.e. because RVFI uses alu_jmpr instead of operand_a_fw)

sva/cv32e40x_id_stage_sva.sv Show resolved Hide resolved
Signed-off-by: Oystein Knauserud <[email protected]>
@Silabs-ArjanB Silabs-ArjanB merged commit 47d4a57 into openhwgroup:master Jul 1, 2022
@Silabs-ArjanB Silabs-ArjanB added the Component:RTL For issues in the RTL (e.g. for files in the rtl directory) label Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants