[rtl] Remove paths between dmem and imem signals

Prior to this change Ibex had multiple feedthrough paths from the data
memory interface to the instruction memory interface. This existed
because Ibex would hold off doing a instruction fetch for a jump or
branch if there was a outstanding memory request. It would wait for the
response to be available so either the jump or branch would occur or an
exception was taken.

With this change the branch or jump will speculatively begin the
instruction fetch whilst there is an outstanding memory request. Should
an exception result from the memory request the fetch will be discarded
and the exception taken as normal.

An alternative fix would not factor the data error response
(data_err_i) directly into the controller logic for branches and jumps.
With this option new stall cycles would be introduced anywhere a branch
or jump immediately follows a memory instruction which would have an
adverse impact on performance.

rtl/ibex_controller.sv

@@ -142,8 +142,9 @@ module ibex_controller #(
   logic illegal_dret;
   logic illegal_umode;
   logic exc_req_lsu;
-  logic special_req_all;
+  logic special_req;
-  logic special_req_branch;
+  logic special_req_pc_change;
+  logic special_req_flush_only;
   logic enter_debug_mode_d;
   logic enter_debug_mode_q;
   logic ebreak_into_debug;
@@ -219,29 +220,18 @@ module ibex_controller #(
 
 
   // special requests: special instructions, pipeline flushes, exceptions...
+  // All terms in these expressions are qualified by instr_valid_i except exc_req_lsu which can come
+  // from the Writeback stage with no instr_valid_i from the ID stage
 
-  // To avoid creating a path from data_err_i -> instr_req_o and to help timing the below
+  // These special requests only cause a pipeline flush and in particular don't cause a PC change
-  // special_req_all has a version that only applies to branches. For a branch the controller needs
+  // that is outside the normal execution flow
-  // to set pc_set_o but only if there is no special request. If the generic special_req_all signal
+  assign special_req_flush_only = wfi_insn | csr_pipe_flush;
-  // is used then a variety of signals that will never cause a special request during a branch
+
-  // instruction end up factored into pc_set_o. The special_req_branch only considers the special
+  // These special requests cause a change in PC
-  // request reasons that are relevant to a branch.
+  assign special_req_pc_change = mret_insn | dret_insn | exc_req_d | exc_req_lsu;
 
   // generic special request signal, applies to all instructions
-  // All terms in this expression are qualified by instr_valid_i except exc_req_lsu which can come
+  assign special_req = special_req_pc_change | special_req_flush_only;
-  // from the Writeback stage with no instr_valid_i from the ID stage
-  assign special_req_all = mret_insn | dret_insn | wfi_insn | csr_pipe_flush |
-      exc_req_d | exc_req_lsu;
-
-  // special request that can specifically occur during branch instructions
-  // All terms in this expression are qualified by instr_valid_i
-  assign special_req_branch = instr_fetch_err & (ctrl_fsm_cs != FLUSH);
-
-  `ASSERT(SpecialReqBranchGivesSpecialReqAll,
-    special_req_branch |-> special_req_all)
-
-  `ASSERT(SpecialReqAllGivesSpecialReqBranchIfBranchInst,
-    special_req_all && (branch_set_i || jump_set_i) |-> special_req_branch)
 
   // Exception/fault prioritisation is taken from Table 3.7 of Priviledged Spec v1.11
   if (WritebackStage) begin : g_wb_exceptions
@@ -486,7 +476,7 @@ module ibex_controller #(
 
 
         // Get ready for special instructions, exceptions, pipeline flushes
-        if (special_req_all) begin
+        if (special_req) begin
           // Halt IF but don't flush ID. This leaves a valid instruction in
           // ID so controller can determine appropriate action in the
           // FLUSH state.
@@ -503,26 +493,24 @@ module ibex_controller #(
           end
         end
 
-        if (!special_req_branch) begin
+        if (branch_set_i || jump_set_i) begin
-          if (branch_set_i || jump_set_i) begin
+          // Only set the PC if the branch predictor hasn't already done the branch for us
-            // Only set the PC if the branch predictor hasn't already done the branch for us
+          pc_set_o       = BranchPredictor ? ~instr_bp_taken_i : 1'b1;
-            pc_set_o       = BranchPredictor ? ~instr_bp_taken_i : 1'b1;
 
-            perf_tbranch_o = branch_set_i;
+          perf_tbranch_o = branch_set_i;
-            perf_jump_o    = jump_set_i;
+          perf_jump_o    = jump_set_i;
-          end
+        end
 
-          if (BranchPredictor) begin
+        if (BranchPredictor) begin
-            if (instr_bp_taken_i & branch_not_set_i) begin
+          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
+            // If the instruction is a branch that was predicted to be taken but was not taken
-              // signal a mispredict.
+            // signal a mispredict.
-              nt_branch_mispredict_o = 1'b1;
+            nt_branch_mispredict_o = 1'b1;
-            end
           end
         end
 
         // pc_set signal excluding branch taken condition
-        if ((branch_set_spec_i || jump_set_i) && !special_req_branch) begin
+        if (branch_set_spec_i || jump_set_i) begin
           // Only speculatively set the PC if the branch predictor hasn't already done the branch
           // for us
           pc_set_spec_o = BranchPredictor ? ~instr_bp_taken_i : 1'b1;
@@ -535,7 +523,7 @@ module ibex_controller #(
           halt_if = 1'b1;
         end
 
-        if (!stall && !special_req_all) begin
+        if (!stall && !special_req) begin
           if (enter_debug_mode_d) begin
             // enter debug mode
             ctrl_fsm_ns = DBG_TAKEN_IF;
@@ -853,4 +841,77 @@ module ibex_controller #(
   // The speculative branch signal should be set whenever the actual branch signal is set
   `ASSERT(IbexSpecImpliesSetPC, pc_set_o |-> pc_set_spec_o)
 
+  `ifdef INC_ASSERT
+    // If something that causes a jump into an exception handler is seen that jump must occur before
+    // the next instruction executes. The logic tracks whether a jump into an exception handler is
+    // expected. Assertions check the jump occurs.
+
+    logic exception_req, exception_req_pending, exception_req_accepted, exception_req_done;
+    logic exception_pc_set, seen_exception_pc_set, expect_exception_pc_set;
+    logic exception_req_needs_pc_set;
+
+    assign exception_req = (special_req | enter_debug_mode_d | handle_irq);
+    // Any exception rquest will cause a transition out of DECODE, once the controller transitions
+    // back into DECODE we're done handling the request.
+    assign exception_req_done =
+      exception_req_pending & (ctrl_fsm_cs != DECODE) & (ctrl_fsm_ns == DECODE);
+
+    assign exception_req_needs_pc_set = enter_debug_mode_d | handle_irq | special_req_pc_change;
+
+    // An exception PC set uses specific PC types
+    assign exception_pc_set =
+      exception_req_pending & (pc_set_o & (pc_mux_o inside {PC_EXC, PC_ERET, PC_DRET}));
+
+    always @(posedge clk_i or negedge rst_ni) begin
+      if (!rst_ni) begin
+        exception_req_pending   <= 1'b0;
+        exception_req_accepted  <= 1'b0;
+        expect_exception_pc_set <= 1'b0;
+        seen_exception_pc_set   <= 1'b0;
+      end else begin
+        // Keep `exception_req_pending` asserted once an exception_req is seen until it is done
+        exception_req_pending <= (exception_req_pending | exception_req) & ~exception_req_done;
+
+        // The exception req has been accepted once the controller transitions out of decode
+        exception_req_accepted <= (exception_req_accepted & ~exception_req_done) |
+          (exception_req & ctrl_fsm_ns != DECODE);
+
+        // Set `expect_exception_pc_set` if exception req needs one and keep it asserted until
+        // exception req is done
+        expect_exception_pc_set <= (expect_exception_pc_set | exception_req_needs_pc_set) &
+          ~exception_req_done;
+
+        // Keep `seen_exception_pc_set` asserted once an exception PC set is seen until the
+        // exception req is done
+        seen_exception_pc_set <= (seen_exception_pc_set | exception_pc_set) & ~exception_req_done;
+      end
+    end
+
+    // Once an exception request has been accepted it must be handled before controller goes back to
+    // DECODE
+    `ASSERT(IbexNoDoubleExceptionReq, exception_req_accepted |-> ctrl_fsm_cs != DECODE)
+
+    // Only signal ready, allowing a new instruction into ID, if there is no exception request
+    // pending or it is done this cycle.
+    `ASSERT(IbexDontSkipExceptionReq,
+      id_in_ready_o |-> !exception_req_pending || exception_req_done)
+
+    // Once a PC set has been performed for an exception request there must not be any other
+    // excepting those to move into debug mode.
+    `ASSERT(IbexNoDoubleSpecialReqPCSet,
+      seen_exception_pc_set &&
+        !((ctrl_fsm_cs inside {DBG_TAKEN_IF, DBG_TAKEN_ID}) &&
+          (pc_mux_o == PC_EXC) && (exc_pc_mux_o == EXC_PC_DBD))
+      |-> !pc_set_o)
+
+    // When an exception request is done there must have been an appropriate PC set (either this
+    // cycle or a previous one).
+    `ASSERT(IbexSetExceptionPCOnSpecialReqIfExpected,
+      exception_req_pending && expect_exception_pc_set && exception_req_done |->
+      seen_exception_pc_set || exception_pc_set)
+
+    // If there's a pending exception req that doesn't need a PC set we must not see one
+    `ASSERT(IbexNoPCSetOnSpecialReqIfNotExpected,
+      exception_req_pending && !expect_exception_pc_set |-> ~pc_set_o)
+  `endif
 endmodule

rtl/ibex_id_stage.sv

@@ -197,15 +197,17 @@ module ibex_id_stage #(
   logic        wb_exception;
 
   logic        branch_in_dec;
-  logic        branch_spec, branch_set_spec;
+  logic        branch_spec, branch_set_spec, branch_set_raw_spec;
-  logic        branch_set, branch_set_d;
+  logic        branch_set, branch_set_raw, branch_set_raw_d;
+  logic        branch_jump_set_done_q, branch_jump_set_done_d;
   logic        branch_not_set;
   logic        branch_taken;
   logic        jump_in_dec;
   logic        jump_set_dec;
-  logic        jump_set;
+  logic        jump_set, jump_set_raw;
 
   logic        instr_first_cycle;
+  logic        instr_executing_spec;
   logic        instr_executing;
   logic        instr_done;
   logic        controller_run;
@@ -651,29 +653,52 @@ module ibex_id_stage #(
   if (BranchTargetALU && !DataIndTiming) begin : g_branch_set_direct
     // Branch set fed straight to controller with branch target ALU
     // (condition pass/fail used same cycle as generated instruction request)
-    assign branch_set      = branch_set_d;
+    assign branch_set_raw      = branch_set_raw_d;
-    assign branch_set_spec = branch_spec;
+    assign branch_set_raw_spec = branch_spec;
   end else begin : g_branch_set_flop
     // Branch set flopped without branch target ALU, or in fixed time execution mode
     // (condition pass/fail used next cycle where branch target is calculated)
-    logic branch_set_q;
+    logic branch_set_raw_q;
 
     always_ff @(posedge clk_i or negedge rst_ni) begin
       if (!rst_ni) begin
-        branch_set_q <= 1'b0;
+        branch_set_raw_q <= 1'b0;
       end else begin
-        branch_set_q <= branch_set_d;
+        branch_set_raw_q <= branch_set_raw_d;
       end
     end
 
     // Branches always take two cycles in fixed time execution mode, with or without the branch
     // target ALU (to avoid a path from the branch decision into the branch target ALU operand
     // muxing).
-    assign branch_set      = (BranchTargetALU && !data_ind_timing_i) ? branch_set_d : branch_set_q;
+    assign branch_set_raw      = (BranchTargetALU && !data_ind_timing_i) ? branch_set_raw_d : branch_set_raw_q;
     // Use the speculative branch signal when BTALU is enabled
-    assign branch_set_spec = (BranchTargetALU && !data_ind_timing_i) ? branch_spec : branch_set_q;
+    assign branch_set_raw_spec = (BranchTargetALU && !data_ind_timing_i) ? branch_spec : branch_set_raw_q;
   end
 
+  // Track whether the current instruction in ID/EX has done a branch or jump set.
+  assign branch_jump_set_done_d = (branch_set_raw | jump_set_raw | branch_jump_set_done_q) &
+    ~instr_valid_clear_o;
+
+  always_ff @(posedge clk_i or negedge rst_ni) begin
+    if (!rst_ni) begin
+      branch_jump_set_done_q <= 1'b0;
+    end else begin
+      branch_jump_set_done_q <= branch_jump_set_done_d;
+    end
+  end
+
+  // the _raw signals from the state machine may be asserted for multiple cycles when
+  // instr_executing_spec is asserted and instr_executing is not asserted. This may occur where
+  // a memory error is seen or a there are outstanding memory accesses (indicate a load or store is
+  // in the WB stage). The branch or jump speculatively begins the fetch but is held back from completing
+  // until it is certain the outstanding access hasn't seen a memory error. This logic ensures only
+  // the first cycle of a branch or jump set is sent to the controller to prevent needless extra IF
+  // flushes and fetches.
+  assign jump_set        = jump_set_raw        & ~branch_jump_set_done_q;
+  assign branch_set      = branch_set_raw      & ~branch_jump_set_done_q;
+  assign branch_set_spec = branch_set_raw_spec & ~branch_jump_set_done_q;
+
   // Branch condition is calculated in the first cycle and flopped for use in the second cycle
   // (only used in fixed time execution mode to determine branch destination).
   if (DataIndTiming) begin : g_sec_branch_taken
@@ -691,7 +716,7 @@ module ibex_id_stage #(
 
   end else begin : g_nosec_branch_taken
 
-    // Signal unused without fixed time execution mode - only taken branches will trigger branch_set
+    // Signal unused without fixed time execution mode - only taken branches will trigger branch_set_raw
     assign branch_taken = 1'b1;
 
   end
@@ -711,9 +736,9 @@ module ibex_id_stage #(
 
   always_ff @(posedge clk_i or negedge rst_ni) begin : id_pipeline_reg
     if (!rst_ni) begin
-      id_fsm_q            <= FIRST_CYCLE;
+      id_fsm_q <= FIRST_CYCLE;
-    end else begin
+    end else if (instr_executing) begin
-      id_fsm_q            <= id_fsm_d;
+      id_fsm_q <= id_fsm_d;
     end
   end
 
@@ -729,13 +754,13 @@ module ibex_id_stage #(
     stall_jump              = 1'b0;
     stall_branch            = 1'b0;
     stall_alu               = 1'b0;
-    branch_set_d            = 1'b0;
+    branch_set_raw_d        = 1'b0;
     branch_spec             = 1'b0;
     branch_not_set          = 1'b0;
-    jump_set                = 1'b0;
+    jump_set_raw            = 1'b0;
     perf_branch_o           = 1'b0;
 
-    if (instr_executing) begin
+    if (instr_executing_spec) begin
       unique case (id_fsm_q)
         FIRST_CYCLE: begin
           unique case (1'b1)
@@ -763,10 +788,10 @@ module ibex_id_stage #(
               // cond branch operation
               // All branches take two cycles in fixed time execution mode, regardless of branch
               // condition.
-              id_fsm_d      = (data_ind_timing_i || (!BranchTargetALU && branch_decision_i)) ?
+              id_fsm_d         = (data_ind_timing_i || (!BranchTargetALU && branch_decision_i)) ?
-                                  MULTI_CYCLE : FIRST_CYCLE;
+                                     MULTI_CYCLE : FIRST_CYCLE;
-              stall_branch  = (~BranchTargetALU & branch_decision_i) | data_ind_timing_i;
+              stall_branch     = (~BranchTargetALU & branch_decision_i) | data_ind_timing_i;
-              branch_set_d  = branch_decision_i | data_ind_timing_i;
+              branch_set_raw_d = (branch_decision_i | data_ind_timing_i);
 
               if (BranchPredictor) begin
                 branch_not_set = ~branch_decision_i;
@@ -781,7 +806,7 @@ module ibex_id_stage #(
               // BTALU means jumps only need one cycle
               id_fsm_d      = BranchTargetALU ? FIRST_CYCLE : MULTI_CYCLE;
               stall_jump    = ~BranchTargetALU;
-              jump_set      = jump_set_dec;
+              jump_set_raw  = jump_set_dec;
             end
             alu_multicycle_dec: begin
               stall_alu     = 1'b1;
@@ -874,14 +899,32 @@ module ibex_id_stage #(
     // - A load/store error
     //   This will cause a precise exception for the instruction in WB so ID/EX instruction must not
     //   execute
+    //
+    // instr_executing_spec is a speculative signal. It indicates an instruction can execute
+    // assuming there are no exceptions from writeback and any outstanding memory access won't
+    // receive an error. It is required so branch and jump requests don't factor in an incoming dmem
+    // error (that in turn would factor directly into imem requests leading to a feedthrough path).
+    //
+    // instr_executing is the full signal, it will only allow execution once any potential
+    // exceptions from writeback have been resolved.
+    assign instr_executing_spec = instr_valid_i      &
+                                  ~instr_fetch_err_i &
+                                  controller_run     &
+                                  ~stall_ld_hz;
+
     assign instr_executing = instr_valid_i              &
                              ~instr_kill                &
                              ~stall_ld_hz               &
                              ~outstanding_memory_access;
 
+    `ASSERT(IbexExecutingSpecIfExecuting, instr_executing |-> instr_executing_spec)
+
     `ASSERT(IbexStallIfValidInstrNotExecuting,
       instr_valid_i & ~instr_kill & ~instr_executing |-> stall_id)
 
+    `ASSERT(IbexCannotRetireWithPendingExceptions,
+      instr_done |-> ~(wb_exception | outstanding_memory_access))
+
     // Stall for reasons related to memory:
     // * There is an outstanding memory access that won't resolve this cycle (need to wait to allow
     //   precise exceptions)
@@ -939,7 +982,8 @@ module ibex_id_stage #(
     assign stall_ld_hz   = 1'b0;
 
     // Without writeback stage any valid instruction that hasn't seen an error will execute
-    assign instr_executing = instr_valid_i & ~instr_fetch_err_i & controller_run;
+    assign instr_executing_spec = instr_valid_i & ~instr_fetch_err_i & controller_run;
+    assign instr_executing = instr_executing_spec;
 
     `ASSERT(IbexStallIfValidInstrNotExecuting,
       instr_valid_i & ~instr_fetch_err_i & ~instr_executing & controller_run |-> stall_id)