retirement: Disable side effects on exception (proposed corrections) (#…

…512)

Co-authored-by: Arusekk <arek_koz@o2.pl>

coreblocks/core.py

@@ -136,6 +136,7 @@ def elaborate(self, platform):
             rf_free=rf.free,
             precommit=self.func_blocks_unifier.get_extra_method(InstructionPrecommitKey()),
             exception_cause_get=self.exception_cause_register.get,
+            frat_rename=frat.rename,
         )
 
         m.submodules.csr_generic = self.csr_generic

coreblocks/lsu/dummyLsu.py

@@ -202,6 +202,7 @@ def elaborate(self, platform):
         valid = Signal()  # current_instr is valid
         execute = Signal()  # start execution
         issued = Signal()  # instruction was issued to the bus
+        flush = Signal()  # exception handling, requests are not issued
         current_instr = Record(self.lsu_layouts.rs.data_layout)
 
         m.submodules.requester = requester = LSURequesterWB(self.gen_params, self.bus)
@@ -238,7 +239,8 @@ def _(reg_id: Value, reg_val: Value):
 
         # Issues load/store requests when the instruction is known, is a LOAD/STORE, and just before commit.
         # Memory loads can be issued speculatively.
-        with Transaction().body(m, request=instr_ready & ~issued & ~instr_is_fence & (execute | instr_is_load)):
+        do_issue = ~issued & instr_ready & ~flush & ~instr_is_fence & (execute | instr_is_load)
+        with Transaction().body(m, request=do_issue):
             m.d.sync += issued.eq(1)
             res = requester.issue(
                 m,
@@ -250,8 +252,9 @@ def _(reg_id: Value, reg_val: Value):
             with m.If(res["exception"]):
                 results.write(m, data=0, exception=res["exception"], cause=res["cause"])
 
-        # Handles FENCE as a no-op.
-        with Transaction().body(m, request=instr_ready & ~issued & instr_is_fence):
+        # Handles FENCE and flushed instructions as a no-op.
+        do_skip = ~issued & (instr_ready & ~flush & instr_is_fence | valid & flush)
+        with Transaction().body(m, request=do_skip):
             m.d.sync += issued.eq(1)
             results.write(m, data=0, exception=0, cause=0)
 
@@ -278,8 +281,10 @@ def _():
             }
 
         @def_method(m, self.precommit)
-        def _(rob_id: Value):
-            with m.If(valid & (rob_id == current_instr.rob_id) & ~instr_is_fence):
+        def _(rob_id: Value, side_fx: Value):
+            with m.If(~side_fx):
+                m.d.comb += flush.eq(1)
+            with m.Elif(valid & (rob_id == current_instr.rob_id) & ~instr_is_fence):
                 m.d.comb += execute.eq(1)
 
         return m

coreblocks/params/layouts.py

@@ -106,6 +106,9 @@ def __init__(self, gen_params: GenParams):
         self.error: LayoutListField = ("error", 1)
         """Request ended with an error."""
 
+        self.side_fx: LayoutListField = ("side_fx", 1)
+        """Side effects are enabled."""
+
 
 class SchedulerLayouts:
     """Layouts used in the scheduler."""
@@ -221,7 +224,7 @@ def __init__(self, gen_params: GenParams):
 
         self.rat_rename_out: LayoutList = [fields.rp_s1, fields.rp_s2]
 
-        self.rat_commit_in: LayoutList = [fields.rl_dst, fields.rp_dst]
+        self.rat_commit_in: LayoutList = [fields.rl_dst, fields.rp_dst, fields.side_fx]
 
         self.rat_commit_out: LayoutList = [self.old_rp_dst]
 
@@ -328,7 +331,7 @@ class RetirementLayouts:
     def __init__(self, gen_params: GenParams):
         fields = gen_params.get(CommonLayoutFields)
 
-        self.precommit: LayoutList = [fields.rob_id]
+        self.precommit: LayoutList = [fields.rob_id, fields.side_fx]
 
 
 class RSLayouts:

coreblocks/stages/retirement.py

@@ -1,10 +1,12 @@
 from amaranth import *
 from coreblocks.params.dependencies import DependencyManager
 from coreblocks.params.keys import GenericCSRRegistersKey
+from coreblocks.params.layouts import CommonLayoutFields
 
 from transactron.core import Method, Transaction, TModule
 from coreblocks.params.genparams import GenParams
 from coreblocks.structs_common.csr_generic import CSRAddress, DoubleCounterCSR
+from transactron.lib.connectors import Forwarder
 
 
 class Retirement(Elaboratable):
@@ -18,7 +20,8 @@ def __init__(
         free_rf_put: Method,
         rf_free: Method,
         precommit: Method,
-        exception_cause_get: Method
+        exception_cause_get: Method,
+        frat_rename: Method,
     ):
         self.gen_params = gen_params
         self.rob_peek = rob_peek
@@ -28,6 +31,7 @@ def __init__(
         self.rf_free = rf_free
         self.precommit = precommit
         self.exception_cause_get = exception_cause_get
+        self.rename = frat_rename
 
         self.instret_csr = DoubleCounterCSR(gen_params, CSRAddress.INSTRET, CSRAddress.INSTRETH)
 
@@ -36,18 +40,30 @@ def elaborate(self, platform):
 
         m.submodules.instret_csr = self.instret_csr
 
+        side_fx = Signal(reset=1)
+        side_fx_comb = Signal()
+
+        m.d.comb += side_fx_comb.eq(side_fx)
+
+        fields = self.gen_params.get(CommonLayoutFields)
+        m.submodules.frat_fix = frat_fix = Forwarder([fields.rl_dst, fields.rp_dst])
+
         with Transaction().body(m):
             # TODO: do we prefer single precommit call per instruction?
             # If so, the precommit method should send an acknowledge signal here.
             # Just calling once is not enough, because the insn might not be in relevant unit yet.
             rob_entry = self.rob_peek(m)
-            self.precommit(m, rob_id=rob_entry.rob_id)
+            self.precommit(m, rob_id=rob_entry.rob_id, side_fx=side_fx)
 
         with Transaction().body(m):
             rob_entry = self.rob_retire(m)
 
             # TODO: Trigger InterruptCoordinator (handle exception) when rob_entry.exception is set.
-            with m.If(rob_entry.exception):
+            with m.If(rob_entry.exception & side_fx):
+                m.d.sync += side_fx.eq(0)
+                m.d.comb += side_fx_comb.eq(0)
+                # TODO: only set mcause/trigger IC if cause is actual exception and not e.g.
+                # misprediction or pipeline flush after some fence.i or changing ISA
                 mcause = self.gen_params.get(DependencyManager).get_dependency(GenericCSRRegistersKey()).mcause
                 cause = self.exception_cause_get(m).cause
                 entry = Signal(self.gen_params.isa.xlen)
@@ -56,14 +72,28 @@ def elaborate(self, platform):
                 mcause.write(m, entry)
 
             # set rl_dst -> rp_dst in R-RAT
-            rat_out = self.r_rat_commit(m, rl_dst=rob_entry.rob_data.rl_dst, rp_dst=rob_entry.rob_data.rp_dst)
+            rat_out = self.r_rat_commit(
+                m, rl_dst=rob_entry.rob_data.rl_dst, rp_dst=rob_entry.rob_data.rp_dst, side_fx=side_fx
+            )
 
-            self.rf_free(m, rat_out.old_rp_dst)
+            rp_freed = Signal(self.gen_params.phys_regs_bits)
+            with m.If(side_fx_comb):
+                m.d.av_comb += rp_freed.eq(rat_out.old_rp_dst)
+                self.instret_csr.increment(m)
+            with m.Else():
+                m.d.av_comb += rp_freed.eq(rob_entry.rob_data.rp_dst)
+                # free the phys_reg with computed value and restore old reg into FRAT as well
+                # FRAT calls are in a separate transaction to avoid locking the rename method
+                frat_fix.write(m, rl_dst=rob_entry.rob_data.rl_dst, rp_dst=rat_out.old_rp_dst)
+
+            self.rf_free(m, rp_freed)
 
             # put old rp_dst to free RF list
-            with m.If(rat_out.old_rp_dst):  # don't put rp0 to free list - reserved to no-return instructions
-                self.free_rf_put(m, rat_out.old_rp_dst)
+            with m.If(rp_freed):  # don't put rp0 to free list - reserved to no-return instructions
+                self.free_rf_put(m, rp_freed)
 
-            self.instret_csr.increment(m)
+        with Transaction().body(m):
+            data = frat_fix.read(m)
+            self.rename(m, rl_s1=0, rl_s2=0, rl_dst=data["rl_dst"], rp_dst=data["rp_dst"])
 
         return m

coreblocks/structs_common/csr.py

@@ -219,13 +219,13 @@ def elaborate(self, platform):
         done = Signal()
         accepted = Signal()
         exception = Signal()
-        rob_sfx_empty = Signal()
+        precommitting = Signal()
 
         current_result = Signal(self.gen_params.isa.xlen)
 
         instr = Record(self.csr_layouts.rs.data_layout + [("valid", 1)])
 
-        m.d.comb += ready_to_process.eq(rob_sfx_empty & instr.valid & (instr.rp_s1 == 0))
+        m.d.comb += ready_to_process.eq(precommitting & instr.valid & (instr.rp_s1 == 0))
 
         # RISCV Zicsr spec Table 1.1
         should_read_csr = Signal()
@@ -251,6 +251,8 @@ def elaborate(self, platform):
         # Temporary, until privileged spec is implemented
         priv_level = Signal(PrivilegeLevel, reset=PrivilegeLevel.MACHINE)
 
+        exe_side_fx = Signal()
+
         # Methods used within this Tranaction are CSRRegister internal _fu_(read|write) handlers which are always ready
         with Transaction().body(m, request=(ready_to_process & ~done)):
             with m.Switch(instr.csr):
@@ -266,7 +268,8 @@ def elaborate(self, platform):
                         with m.If(priv_valid):
                             read_val = Signal(self.gen_params.isa.xlen)
                             with m.If(should_read_csr & ~done):
-                                m.d.comb += read_val.eq(read(m))
+                                with m.If(exe_side_fx):
+                                    m.d.comb += read_val.eq(read(m))
                                 m.d.sync += current_result.eq(read_val)
 
                             if read_only:
@@ -283,7 +286,8 @@ def elaborate(self, platform):
                                             m.d.comb += write_val.eq(read_val | instr.s1_val)
                                         with m.Case(Funct3.CSRRC, Funct3.CSRRCI):
                                             m.d.comb += write_val.eq(read_val & (~instr.s1_val))
-                                    write(m, write_val)
+                                    with m.If(exe_side_fx):
+                                        write(m, write_val)
 
                         with m.Else():
                             # Missing privilege
@@ -338,10 +342,12 @@ def _():
         def _():
             return {"from_pc": instr.pc, "next_pc": instr.pc + self.gen_params.isa.ilen_bytes}
 
-        # Generate rob_sfx_empty signal from precommit
+        # Generate precommitting signal from precommit
         @def_method(m, self.precommit)
-        def _(rob_id):
-            m.d.comb += rob_sfx_empty.eq(instr.rob_id == rob_id)
+        def _(rob_id: Value, side_fx: Value):
+            with m.If(instr.rob_id == rob_id):
+                m.d.comb += precommitting.eq(1)
+                m.d.comb += exe_side_fx.eq(side_fx)
 
         return m
 

coreblocks/structs_common/rat.py

@@ -42,8 +42,9 @@ def elaborate(self, platform):
         m = TModule()
 
         @def_method(m, self.commit)
-        def _(rp_dst: Value, rl_dst: Value):
-            m.d.sync += self.entries[rl_dst].eq(rp_dst)
+        def _(rp_dst: Value, rl_dst: Value, side_fx: Value):
+            with m.If(side_fx):
+                m.d.sync += self.entries[rl_dst].eq(rp_dst)
             return {"old_rp_dst": self.entries[rl_dst]}
 
         return m

test/asm/exception.asm

@@ -0,0 +1,5 @@
+li x2, 2
+li x1, 1
+.4byte 0  /* should be unimp, but it would test nothing since unimp is system and stalls the fetcher >:( */
+li x2, 9
+

test/asm/exception_mem.asm

@@ -0,0 +1,11 @@
+# Data adress space:
+# 0x0 - one
+# 0x4 - two
+li x1, 1
+sw x1, 0(x0)
+li x2, 2
+sw x2, 4(x0)
+.4byte 0  /* should be unimp, but it would test nothing since unimp is system and stalls the fetcher >:( */
+sw x1, 4(x0)  /* TODO: actually check the side fx */
+li x2, 9
+

test/lsu/test_dummylsu.py

@@ -431,7 +431,7 @@ def precommiter(self):
             while len(self.precommit_data) == 0:
                 yield
             rob_id = self.precommit_data[-1]  # precommit is called continously until instruction is retired
-            yield from self.test_module.precommit.call(rob_id=rob_id)
+            yield from self.test_module.precommit.call(rob_id=rob_id, side_fx=1)
 
     def test(self):
         @def_method_mock(lambda: self.test_module.exception_report)

test/stages/test_retirement.py

@@ -3,7 +3,7 @@
 from coreblocks.structs_common.csr_generic import GenericCSRRegisters
 
 from transactron.lib import FIFO, Adapter
-from coreblocks.structs_common.rat import RRAT
+from coreblocks.structs_common.rat import FRAT, RRAT
 from coreblocks.params import ROBLayouts, RFLayouts, GenParams, LSULayouts, SchedulerLayouts
 from coreblocks.params.configurations import test_core_config
 
@@ -26,6 +26,7 @@ def elaborate(self, platform):
         exception_layouts = self.gen_params.get(ExceptionRegisterLayouts)
 
         m.submodules.r_rat = self.rat = RRAT(gen_params=self.gen_params)
+        m.submodules.f_rat = self.frat = FRAT(gen_params=self.gen_params)
         m.submodules.free_rf_list = self.free_rf = FIFO(
             scheduler_layouts.free_rf_layout, 2**self.gen_params.phys_regs_bits
         )
@@ -51,6 +52,7 @@ def elaborate(self, platform):
             rf_free=self.mock_rf_free.adapter.iface,
             precommit=self.mock_precommit.adapter.iface,
             exception_cause_get=self.mock_exception_cause.adapter.iface,
+            frat_rename=self.frat.rename,
         )
 
         m.submodules.free_rf_fifo_adapter = self.free_rf_adapter = TestbenchIO(AdapterTrans(self.free_rf.read))
@@ -123,7 +125,7 @@ def rf_free_process(reg_id):
             self.assertEqual(reg_id, self.rf_free_q.popleft())
 
         @def_method_mock(lambda: retc.mock_precommit, sched_prio=2)
-        def precommit_process(rob_id):
+        def precommit_process(rob_id, side_fx):
             self.assertEqual(rob_id, self.precommit_q.popleft())
 
         @def_method_mock(lambda: retc.mock_exception_cause)

test/structs_common/test_csr.py

@@ -130,7 +130,7 @@ def process_test(self):
                 yield from self.dut.update.call(reg_id=op["exp"]["rs1"]["rp_s1"], reg_val=op["exp"]["rs1"]["value"])
 
             yield from self.random_wait()
-            yield from self.dut.precommit.call()
+            yield from self.dut.precommit.call(side_fx=1)
 
             yield from self.random_wait()
             res = yield from self.dut.accept.call()
@@ -183,7 +183,7 @@ def process_exception_test(self):
             )
 
             yield from self.random_wait()
-            yield from self.dut.precommit.call(rob_id=rob_id)
+            yield from self.dut.precommit.call(rob_id=rob_id, side_fx=1)
 
             yield from self.random_wait()
             res = yield from self.dut.accept.call()

test/test_core.py

@@ -243,6 +243,8 @@ def test_randomized(self):
         ("fibonacci", "fibonacci.asm", 1200, {2: 2971215073}, basic_core_config),
         ("fibonacci_mem", "fibonacci_mem.asm", 610, {3: 55}, basic_core_config),
         ("csr", "csr.asm", 200, {1: 1, 2: 4}, full_core_config),
+        ("exception", "exception.asm", 200, {1: 1, 2: 2}, basic_core_config),
+        ("exception_mem", "exception_mem.asm", 200, {1: 1, 2: 2}, basic_core_config),
     ],
 )
 class TestCoreAsmSource(TestCoreBase):