From b4a688103f256fcd5ffbc5c06b00712ce1de9fa6 Mon Sep 17 00:00:00 2001 From: Arusekk Date: Sun, 29 Oct 2023 00:34:54 +0200 Subject: [PATCH 1/2] retirement: Disable side effects on exception --- coreblocks/core.py | 1 + coreblocks/lsu/dummyLsu.py | 11 ++++++++--- 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, 81 insertions(+), 25 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 409ab3238..4da2a62eb 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 497ef3355..3630d5d9e 100644 --- a/coreblocks/lsu/dummyLsu.py +++ b/coreblocks/lsu/dummyLsu.py @@ -219,7 +219,10 @@ def elaborate(self, platform): m.submodules.internal = internal = LSUDummyInternals(self.gen_params, self.bus, current_instr) - result_ready = internal.result_ready | ((current_instr.exec_fn.op_type == OpType.FENCE) & current_instr.valid) + result_ready = Signal() + m.d.comb += result_ready.eq( + internal.result_ready | ((current_instr.exec_fn.op_type == OpType.FENCE) & current_instr.valid) + ) @def_method(m, self.select, ~reserved) def _(): @@ -256,8 +259,10 @@ def _(): } @def_method(m, self.precommit) - def _(rob_id: Value): - with m.If( + def _(rob_id: Value, side_fx: Value): + with m.If(~side_fx): + m.d.comb += result_ready.eq(1) + with m.Elif( current_instr.valid & (rob_id == current_instr.rob_id) & (current_instr.exec_fn.op_type != OpType.FENCE) ): m.d.comb += internal.execute.eq(1) diff --git a/coreblocks/params/layouts.py b/coreblocks/params/layouts.py index 9386d11e0..3b6916285 100644 --- a/coreblocks/params/layouts.py +++ b/coreblocks/params/layouts.py @@ -111,7 +111,11 @@ def __init__(self, gen_params: GenParams): ] self.rat_rename_out = [("rp_s1", gen_params.phys_regs_bits), ("rp_s2", gen_params.phys_regs_bits)] - self.rat_commit_in = [("rl_dst", gen_params.isa.reg_cnt_log), ("rp_dst", gen_params.phys_regs_bits)] + self.rat_commit_in = [ + ("rl_dst", gen_params.isa.reg_cnt_log), + ("rp_dst", gen_params.phys_regs_bits), + ("side_fx", 1), + ] self.rat_commit_out = [("old_rp_dst", gen_params.phys_regs_bits)] @@ -175,6 +179,7 @@ class RetirementLayouts: def __init__(self, gen_params: GenParams): self.precommit = [ ("rob_id", gen_params.rob_entries_bits), + ("side_fx", 1), ] 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 0f8eba862..1d9d74ec3 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 d269049a3..d51edddc1 100644 --- a/test/lsu/test_dummylsu.py +++ b/test/lsu/test_dummylsu.py @@ -430,7 +430,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 d1b1e64e4..4cb8eb565 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(tag=op["exp"]["rs1"]["rp_s1"], value=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 aa03c64e3..28f3a9196 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 18cdbd944a03f8ce49d6a3473f4888fb5936fadb Mon Sep 17 00:00:00 2001 From: Arusekk Date: Mon, 6 Nov 2023 07:21:13 +0100 Subject: [PATCH 2/2] 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