Add speculative loads support #3188

LorenzoR01 · 2026-01-19T12:51:29Z

This PR introduces support for speculative loads, allowing dual issue of a control flow instruction and a load instruction.
Speculative loads improve Coremark performance by up to 7% with no impact on the CVA6 timing.
We have successfully tested the code on Linux and Splash3 benchmarks.

numero-744

Thanks for the contribution!

Unfortunately, I cannot review LSU's FSM as I am not familiar enough with it to understand the internal changes. I don't know enough about the TLB either.
Still, I'm wondering: in the execute stage, the branch unit answers ~~immediately~~ combinatorially, so do we need to go to a new state in LSU's FSM?

Is there a maximum frequency regression due to the ALU->BRANCH->LSU path, or is the logic of the FSM shallow enough?
How does the performance of this PR compare with the performance of the fast but wrong implementation where we only remove the stall condition from the issue stage?

Below are minor comments about the code.

Cc @Gchauvon

numero-744 · 2026-02-01T19:57:11Z

core/commit_stage.sv

    output logic commit_lsu_o,
    // Commit buffer of LSU is ready - EX_STAGE
    input logic commit_lsu_ready_i,
    // Transaction id of first commit port - ID_STAGE


The comment does not match the declaration anymore, we should update it.

This comment is not accurate anymore

numero-744 · 2026-02-01T20:02:45Z

core/issue_stage.sv

-    output logic [5:0] orig_instr_aes_bits
+    output logic [5:0] orig_instr_aes_bits,
+    // Signals speculative loads to LSU for non-idempotent load handling - EX_STAGE
+    output logic speculative_load_o


speculative_load_o should be inserted just after lsu_valid_o IMO

numero-744 · 2026-02-01T20:03:07Z

core/issue_stage.sv

      .rvfi_rs2_o              (rvfi_rs2_o),
-      .orig_instr_aes_bits     (orig_instr_aes_bits)
+      .orig_instr_aes_bits     (orig_instr_aes_bits),
+      .speculative_load_o


speculative_load_o should be inserted just after lsu_valid_o IMO

numero-744 · 2026-02-01T20:04:46Z

core/cva6.sv

      .rvfi_rs2_o           (rvfi_rs2),
-      .orig_instr_aes_bits  (orig_instr_aes)
+      .orig_instr_aes_bits  (orig_instr_aes),
+      .speculative_load_o   (speculative_load)


speculative_load_o should be inserted just after lsu_valid_o IMO

numero-744 · 2026-02-01T20:06:17Z

core/cva6.sv

      .lsu_commit_i            (lsu_commit_commit_ex),           // from commit
      .lsu_commit_ready_o      (lsu_commit_ready_ex_commit),     // to commit
      .commit_tran_id_i        (lsu_commit_trans_id),            // from commit
+      .speculative_load_i      (speculative_load),


speculative_load_i should be inserted just after lsu_valid_i IMO

It is connected to issue stage, not commit stage, right?

numero-744 · 2026-02-01T20:27:19Z

core/load_store_unit.sv

+    // Signals speculative loads for non-idempotent load handling - ISSUE_STAGE
+    input logic speculative_load_i,
+    // Result from branch unit - EX_STAGE
+    input bp_resolve_t resolved_branch_i,


Does this new input add a critical path inside the execute stage?

numero-744 · 2026-02-01T20:30:48Z

core/ex_stage.sv

      .commit_i              (lsu_commit_i),
      .commit_ready_o        (lsu_commit_ready_o),
      .commit_tran_id_i,
+      .speculative_load_i,


speculative_load_i should be inserted just after lsu_valid_i IMO

It is connected to issue stage, not commit stage, right?

numero-744 · 2026-02-01T20:31:40Z

core/load_store_unit.sv

-
+    input logic [CVA6Cfg.NrCommitPorts-1:0][CVA6Cfg.TRANS_ID_BITS-1:0] commit_tran_id_i,
+    // Signals speculative loads for non-idempotent load handling - ISSUE_STAGE
+    input logic speculative_load_i,


speculative_load_i should be inserted just after lsu_valid_i IMO

It is connected to issue stage, not commit stage, right?

numero-744 · 2026-02-01T20:34:03Z

core/load_unit.sv

    input logic page_offset_matches_i,
    // Store buffer is empty - STORE_UNIT
    input logic store_buffer_empty_i,
    // Transaction ID of the committing instruction - COMMIT_STAGE


Suggested change

// Transaction ID of the committing instruction - COMMIT_STAGE

// Transaction ID of the committing instructions - COMMIT_STAGE

numero-744 · 2026-02-01T20:41:06Z

core/load_unit.sv

+  end
  assign inflight_stores = (!dcache_wbuffer_not_ni_i || !store_buffer_empty_i);
-  assign stall_ni = (inflight_stores || not_commit_time) && (paddr_ni && CVA6Cfg.NonIdemPotenceEn);
+  assign stall_ni = (inflight_stores || &not_commit_time) && (paddr_ni && CVA6Cfg.NonIdemPotenceEn);


Previously, the not_commit_time guaranteed that there was no previous instructions as it could be a branch miss or a exception.
Here &not_commit_time = 1'b0 if the load is the second committing instruction, so there might be an uncommitted instruction before it.

I do not yet understand why is this change needed (e.g.: correctness or performance?) and why it is correct.

No this change is not needed for speculative loads and now I also realized that this is wrong, so I'm removing this part.

Okay, I'm curious to know whether removing this part of the diff changes performance

I removed this part, performance are still the same

LorenzoR01 · 2026-02-02T13:28:36Z

Thanks for the contribution!

Unfortunately, I cannot review LSU's FSM as I am not familiar enough with it to understand the internal changes. I don't know enough about the TLB either. Still, I'm wondering: in the execute stage, the branch unit answers ~~immediately~~ combinatorially, so do we need to go to a new state in LSU's FSM?

Is there a maximum frequency regression due to the ALU->BRANCH->LSU path, or is the logic of the FSM shallow enough? How does the performance of this PR compare with the performance of the fast but wrong implementation where we only remove the stall condition from the issue stage?

Below are minor comments about the code.

Cc @Gchauvon

It should be possible to use the branch result combinationally to control the load unit FSM without a maximum frequency regression, however this combinational path will be longer than the one in this PR, since here the branch result is only used to update a register inside the lsu_bypass.

This PR checks the branch result in the next cycle (and there is a new state in the load unit FSM to do this) so it should have a small performance disadvantage with respect to the incorrect implementation where only the stall condition from the issue stage is removed. However, in my testing I didn’t find any significant performance difference between these two implementations; both achieve around +7% CoreMark/MHz. I also observed the same performance results when comparing this PR to an implementation where the branch result is used to control the load unit FSM combinationally.

numero-744 · 2026-02-03T20:08:04Z

core/ex_stage.sv

        lsu_data  = fu_data_i[1];
        lsu_tinst = tinst_i[1];
+        if (CVA6Cfg.SpeculativeSb) begin
+          speculative_load = branch_valid_i[0] ? 1'b1 : 1'b0;


Suggested change

speculative_load = branch_valid_i[0] ? 1'b1 : 1'b0;

speculative_load = branch_valid_i[0];

Let's avoid the "if true then true else false" tautology

numero-744 · 2026-02-04T15:13:23Z

LGTM except for the load unit FSM that I cannot review

Gchauvon · 2026-02-04T16:09:48Z

The LSU looks good to me. Have you done some ASIC or FPGA synthesis ? Have you been able to test it with the linux boot on FPGA ?

LorenzoR01 · 2026-02-05T07:33:24Z

The LSU looks good to me. Have you done some ASIC or FPGA synthesis ? Have you been able to test it with the linux boot on FPGA ?

Yes, I tested Linux boot on FPGA and ran an ASIC synthesis to confirm that the critical path is unaffected by these changes.

Gchauvon · 2026-02-05T08:48:13Z

Great ! Well it's ok for me.
Cc @JeanRochCoulon

LorenzoR01 requested a review from JeanRochCoulon as a code owner January 19, 2026 12:51

LorenzoR01 force-pushed the lr/spld-rebase branch 2 times, most recently from 340290c to 9bc67a2 Compare January 30, 2026 11:04

Add speculative loads support

04cc91e

LorenzoR01 force-pushed the lr/spld-rebase branch from 9bc67a2 to 04cc91e Compare January 30, 2026 14:56

numero-744 reviewed Feb 1, 2026

View reviewed changes

LorenzoR01 added 4 commits February 3, 2026 13:10

Restore correct functionality of not_commit_time

06e2acb

Move speculative loads detection in execute stage

230f32e

Add suggested changes to code format

69b0c9d

fix not_commit_time size

68f688d

numero-744 reviewed Feb 3, 2026

View reviewed changes

Remove unnecessary mux

74c3830

	// Transaction ID of the committing instruction - COMMIT_STAGE
	// Transaction ID of the committing instructions - COMMIT_STAGE

	speculative_load = branch_valid_i[0] ? 1'b1 : 1'b0;
	speculative_load = branch_valid_i[0];

Add speculative loads support #3188

Are you sure you want to change the base?

Add speculative loads support #3188

Conversation

LorenzoR01 commented Jan 19, 2026

Uh oh!

numero-744 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorenzoR01 commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

numero-744 commented Feb 4, 2026

Uh oh!

Gchauvon commented Feb 4, 2026

Uh oh!

LorenzoR01 commented Feb 5, 2026

Uh oh!

Gchauvon commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants