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

remove branch predictor #49

Merged
merged 7 commits into from Jul 20, 2023

Conversation

christian-herber-nxp
Copy link

No description provided.

@christian-herber-nxp
Copy link
Author

@davideschiavone : Could you please review?

`ASSERT(NoPredictSkid, instr_skid_valid_q |-> ~predict_branch_taken)
`ASSERT(NoPredictIllegal, predict_branch_taken |-> ~illegal_c_insn)
end else begin : g_no_branch_predictor
begin : g_no_branch_predictor
Copy link

Choose a reason for hiding this comment

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

⚠️ [verible-verilog-lint] reported by reviewdog 🐶
Module-level begin-end blocks are not LRM-valid syntax. [Style: floating-begin-end-blocks] [module-begin-block]

`endif

end else begin : g_no_branch_predictor_asserts
begin : g_no_branch_predictor_asserts
Copy link

Choose a reason for hiding this comment

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

⚠️ [verible-verilog-lint] reported by reviewdog 🐶
Module-level begin-end blocks are not LRM-valid syntax. [Style: floating-begin-end-blocks] [module-begin-block]

if (instr_bp_taken_i & branch_not_set_i) begin
// If the instruction is a branch that was predicted to be taken but was not taken
// signal a mispredict.
nt_branch_mispredict_o = 1'b1;

Choose a reason for hiding this comment

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

this signal is always 0, shall we just remove it from the cve2_controller and wherever it is used in the id_stage etc?

Copy link

Choose a reason for hiding this comment

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

Redundant signals were pruned.

if (BranchPredictor) begin : g_calc_nt_addr
assign nt_branch_addr_o = pc_id_i + (instr_is_compressed_i ? 32'd2 : 32'd4);
end else begin : g_n_calc_nt_addr
assign nt_branch_addr_o = 32'd0;

Choose a reason for hiding this comment

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

similarly to the previous comment, shall we just remove this signal and its leaf?

@christian-herber-nxp christian-herber-nxp force-pushed the feature/remove_BranchPredictor branch 2 times, most recently from edf92ee to 0ced299 Compare March 3, 2023 16:12
@szbieg szbieg force-pushed the feature/remove_BranchPredictor branch 2 times, most recently from 05173cf to 36fdcaf Compare June 1, 2023 17:28
@davideschiavone
Copy link

can you @szbieg merge in again the main branch (I made some fixes for verilator)

Also let's removed unused/undeclered signals (or declare them if you use them) as:

I copy here the Verilator errors

ERROR: %Warning-IMPLICIT: ../../../rtl/cve2_if_stage.sv:261:10: Signal definition not found, creating implicitly: 'if_instr_valid'
                                                       : ... Suggested alternative: 'if_instr_err'
  261 |   assign if_instr_valid = fetch_valid;
      |          ^~~~~~~~~~~~~~
                   ... For warning description see https://verilator.org/warn/IMPLICIT?v=4.210
                   ... Use "/* verilator lint_off IMPLICIT */" and lint_on around source to disable this message.
%Warning-IMPLICIT: ../../../rtl/cve2_if_stage.sv:263:10: Signal definition not found, creating implicitly: 'if_instr_addr'
                                                       : ... Suggested alternative: 'if_instr_err'
  263 |   assign if_instr_addr  = fetch_addr;
      |          ^~~~~~~~~~~~~
%Warning-IMPLICIT: ../../../rtl/cve2_if_stage.sv:264:10: Signal definition not found, creating implicitly: 'if_instr_bus_err'
                                                       : ... Suggested alternative: 'if_instr_pmp_err'
  264 |   assign if_instr_bus_err = fetch_err;
      |          ^~~~~~~~~~~~~~~~
%Warning-WIDTH: ../../../rtl/cve2_if_stage.sv:263:25: Operator ASSIGNW expects 1 bits on the Assign RHS, but Assign RHS's VARREF 'fetch_addr' generates 32 bits.
                                                    : ... In instance cve2_top.u_cve2_core.if_stage_i
  263 |   assign if_instr_addr  = fetch_addr;
      |                         ^
%Warning-UNUSED: ../../../rtl/cve2_if_stage.sv:98:22: Bits of signal are not used: 'if_instr_rdata'[31:16]
                                                    : ... In instance cve2_top.u_cve2_core.if_stage_i
   98 |   logic       [31:0] if_instr_rdata;
      |                      ^~~~~~~~~~~~~~
%Warning-UNUSED: ../../../rtl/cve2_if_stage.sv:261:10: Signal is not used: 'if_instr_valid'
                                                     : ... In instance cve2_top.u_cve2_core.if_stage_i
  261 |   assign if_instr_valid = fetch_valid;
      |          ^~~~~~~~~~~~~~
%Warning-UNUSED: ../../../rtl/cve2_if_stage.sv:263:10: Signal is not used: 'if_instr_addr'
                                                     : ... In instance cve2_top.u_cve2_core.if_stage_i
  263 |   assign if_instr_addr  = fetch_addr;
      |          ^~~~~~~~~~~~~
%Warning-UNUSED: ../../../rtl/cve2_if_stage.sv:264:10: Signal is not used: 'if_instr_bus_err'
                                                     : ... In instance cve2_top.u_cve2_core.if_stage_i
  264 |   assign if_instr_bus_err = fetch_err;
      |          ^~~~~~~~~~~~~~~~
%Warning-UNUSED: ../../../rtl/cve2_controller.sv:62:33: Signal is not used: 'branch_not_set_i'
                                                      : ... In instance cve2_top.u_cve2_core.id_stage_i.controller_i
   62 |   input  logic                  branch_not_set_i,         
      |                                 ^~~~~~~~~~~~~~~~

@szbieg szbieg force-pushed the feature/remove_BranchPredictor branch 2 times, most recently from 3bd6ede to b36a71e Compare June 5, 2023 10:55
Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
…ations

Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
Signed-off-by: Szymon Bieganski <szymon.bieganski@oss.nxp.com>
@szbieg szbieg force-pushed the feature/remove_BranchPredictor branch from b36a71e to 8c00895 Compare July 20, 2023 13:54
@davideschiavone davideschiavone changed the title Feature/remove branch predictor remove branch predictor Jul 20, 2023
@davideschiavone davideschiavone merged commit 066ff47 into openhwgroup:main Jul 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants