From 4967501bb526d36895ed19c0bcd146ba1de6e151 Mon Sep 17 00:00:00 2001 From: Arusekk Date: Wed, 15 Nov 2023 11:11:53 +0100 Subject: [PATCH 1/4] retirement: Disable side effects on exception Co-authored-by: Marek Materzok --- coreblocks/core.py | 1 + coreblocks/lsu/dummyLsu.py | 15 ++++++++++----- coreblocks/params/layouts.py | 7 +++++-- coreblocks/stages/retirement.py | 32 +++++++++++++++++++++++++------- coreblocks/structs_common/csr.py | 20 +++++++++++++------- coreblocks/structs_common/rat.py | 5 +++-- test/asm/exception.asm | 5 +++++ test/asm/exception_mem.asm | 11 +++++++++++ test/lsu/test_dummylsu.py | 2 +- test/stages/test_retirement.py | 6 ++++-- test/structs_common/test_csr.py | 4 ++-- test/test_core.py | 2 ++ 12 files changed, 82 insertions(+), 28 deletions(-) create mode 100644 test/asm/exception.asm create mode 100644 test/asm/exception_mem.asm diff --git a/coreblocks/core.py b/coreblocks/core.py index 2adcbbe53..b0c6257db 100644 --- a/coreblocks/core.py +++ b/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 diff --git a/coreblocks/lsu/dummyLsu.py b/coreblocks/lsu/dummyLsu.py index 9047fc542..562c76988 100644 --- a/coreblocks/lsu/dummyLsu.py +++ b/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 diff --git a/coreblocks/params/layouts.py b/coreblocks/params/layouts.py index 5952889eb..7e13b74b2 100644 --- a/coreblocks/params/layouts.py +++ b/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: diff --git a/coreblocks/stages/retirement.py b/coreblocks/stages/retirement.py index f8f4aeac7..a15415ce1 100644 --- a/coreblocks/stages/retirement.py +++ b/coreblocks/stages/retirement.py @@ -18,7 +18,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 +29,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 +38,23 @@ def elaborate(self, platform): m.submodules.instret_csr = self.instret_csr + side_fx = Signal(reset=1) + 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) + # 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,13 +63,24 @@ 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 + ) + + rp_freed = Signal(self.gen_params.phys_regs_bits) + with m.If(side_fx): + m.d.comb += rp_freed.eq(rat_out.old_rp_dst) + with m.Else(): + m.d.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 + # TODO: are method priorities enough? + self.rename(m, rl_s1=0, rl_s2=0, rl_dst=rob_entry.rob_data.rl_dst, rp_dst=rat_out.old_rp_dst) - self.rf_free(m, 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) diff --git a/coreblocks/structs_common/csr.py b/coreblocks/structs_common/csr.py index f1a6d89f1..d7d5419f2 100644 --- a/coreblocks/structs_common/csr.py +++ b/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 diff --git a/coreblocks/structs_common/rat.py b/coreblocks/structs_common/rat.py index 4902c1b99..1ed3f1a8f 100644 --- a/coreblocks/structs_common/rat.py +++ b/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 diff --git a/test/asm/exception.asm b/test/asm/exception.asm new file mode 100644 index 000000000..e3840fc8a --- /dev/null +++ b/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 + diff --git a/test/asm/exception_mem.asm b/test/asm/exception_mem.asm new file mode 100644 index 000000000..c3556e795 --- /dev/null +++ b/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 + diff --git a/test/lsu/test_dummylsu.py b/test/lsu/test_dummylsu.py index 4ac3c059a..a04037be8 100644 --- a/test/lsu/test_dummylsu.py +++ b/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) diff --git a/test/stages/test_retirement.py b/test/stages/test_retirement.py index 05f4cd411..30687a77c 100644 --- a/test/stages/test_retirement.py +++ b/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) diff --git a/test/structs_common/test_csr.py b/test/structs_common/test_csr.py index d7fac6dc9..ffcdaf74a 100644 --- a/test/structs_common/test_csr.py +++ b/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() diff --git a/test/test_core.py b/test/test_core.py index ed5efb337..f5a1d7a1c 100644 --- a/test/test_core.py +++ b/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): From 26fb44b8c31c2d8ace8477ca3da7ee9671c4acee Mon Sep 17 00:00:00 2001 From: Arusekk Date: Mon, 6 Nov 2023 07:21:13 +0100 Subject: [PATCH 2/4] Do not count ignored insns as retired --- coreblocks/stages/retirement.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/coreblocks/stages/retirement.py b/coreblocks/stages/retirement.py index a15415ce1..cab22814c 100644 --- a/coreblocks/stages/retirement.py +++ b/coreblocks/stages/retirement.py @@ -70,6 +70,7 @@ def elaborate(self, platform): rp_freed = Signal(self.gen_params.phys_regs_bits) with m.If(side_fx): m.d.comb += rp_freed.eq(rat_out.old_rp_dst) + self.instret_csr.increment(m) with m.Else(): m.d.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 @@ -82,6 +83,4 @@ def elaborate(self, platform): 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) - return m From f6c0aff36a122e7ff0500b018980130be61b8560 Mon Sep 17 00:00:00 2001 From: Marek Materzok Date: Wed, 15 Nov 2023 12:39:55 +0100 Subject: [PATCH 3/4] Fix side-fx communication --- coreblocks/stages/retirement.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/coreblocks/stages/retirement.py b/coreblocks/stages/retirement.py index cab22814c..52d77f5ec 100644 --- a/coreblocks/stages/retirement.py +++ b/coreblocks/stages/retirement.py @@ -39,6 +39,9 @@ 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) with Transaction().body(m): # TODO: do we prefer single precommit call per instruction? @@ -53,6 +56,7 @@ def elaborate(self, platform): # TODO: Trigger InterruptCoordinator (handle exception) when rob_entry.exception is set. 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 @@ -68,11 +72,11 @@ def elaborate(self, platform): ) rp_freed = Signal(self.gen_params.phys_regs_bits) - with m.If(side_fx): - m.d.comb += rp_freed.eq(rat_out.old_rp_dst) + 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.comb += rp_freed.eq(rob_entry.rob_data.rp_dst) + 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 # TODO: are method priorities enough? self.rename(m, rl_s1=0, rl_s2=0, rl_dst=rob_entry.rob_data.rl_dst, rp_dst=rat_out.old_rp_dst) From 955bea195eff9a8280789101b03fd0b707ed8f88 Mon Sep 17 00:00:00 2001 From: Marek Materzok Date: Wed, 15 Nov 2023 14:31:06 +0100 Subject: [PATCH 4/4] Avoid locking the rename method in retirement --- coreblocks/stages/retirement.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/coreblocks/stages/retirement.py b/coreblocks/stages/retirement.py index 52d77f5ec..28ac8983f 100644 --- a/coreblocks/stages/retirement.py +++ b/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): @@ -43,6 +45,9 @@ def elaborate(self, platform): 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. @@ -78,8 +83,8 @@ def elaborate(self, platform): 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 - # TODO: are method priorities enough? - self.rename(m, rl_s1=0, rl_s2=0, rl_dst=rob_entry.rob_data.rl_dst, rp_dst=rat_out.old_rp_dst) + # 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) @@ -87,4 +92,8 @@ def elaborate(self, platform): with m.If(rp_freed): # don't put rp0 to free list - reserved to no-return instructions self.free_rf_put(m, rp_freed) + 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