From 255abce16ee072ee4fd8063244b161dbe12a3165 Mon Sep 17 00:00:00 2001 From: piotro888 Date: Sun, 12 May 2024 14:37:18 +0200 Subject: [PATCH] Fix fetch resume (#681) --- coreblocks/backend/retirement.py | 2 +- coreblocks/core.py | 6 +-- coreblocks/frontend/fetch/fetch.py | 79 +++++++++++++++++++++++++---- coreblocks/func_blocks/csr/csr.py | 7 ++- coreblocks/func_blocks/fu/priv.py | 2 +- coreblocks/interface/layouts.py | 2 +- coreblocks/params/configurations.py | 4 ++ coreblocks/params/genparams.py | 2 + test/frontend/test_fetch.py | 16 +++--- transactron/core/method.py | 2 +- 10 files changed, 93 insertions(+), 29 deletions(-) diff --git a/coreblocks/backend/retirement.py b/coreblocks/backend/retirement.py index 2fa251ec1..f065ad0e5 100644 --- a/coreblocks/backend/retirement.py +++ b/coreblocks/backend/retirement.py @@ -210,7 +210,7 @@ def flush_instr(rob_entry): resume_pc = Mux(continue_pc_override, continue_pc, handler_pc) m.d.sync += continue_pc_override.eq(0) - self.fetch_continue(m, pc=resume_pc, resume_from_exception=1) + self.fetch_continue(m, pc=resume_pc) # Release pending trap state - allow accepting new reports self.exception_cause_clear(m) diff --git a/coreblocks/core.py b/coreblocks/core.py index 89ea629d3..531f8920a 100644 --- a/coreblocks/core.py +++ b/coreblocks/core.py @@ -118,10 +118,10 @@ def elaborate(self, platform): m.submodules.exception_cause_register = self.exception_cause_register - fetch_resume, fetch_resume_unifiers = self.connections.get_dependency(FetchResumeKey()) + fetch_resume_fb, fetch_resume_unifiers = self.connections.get_dependency(FetchResumeKey()) m.submodules.fetch_resume_unifiers = ModuleConnector(**fetch_resume_unifiers) - m.submodules.fetch_resume_connector = ConnectTrans(fetch_resume, self.frontend.fetch.resume) + m.submodules.fetch_resume_connector = ConnectTrans(fetch_resume_fb, self.frontend.fetch.resume_from_unsafe) m.submodules.announcement = self.announcement m.submodules.func_blocks_unifier = self.func_blocks_unifier @@ -136,7 +136,7 @@ def elaborate(self, platform): exception_cause_get=self.exception_cause_register.get, exception_cause_clear=self.exception_cause_register.clear, frat_rename=frat.rename, - fetch_continue=self.frontend.fetch.resume, + fetch_continue=self.frontend.fetch.resume_from_exception, instr_decrement=self.frontend.core_counter.decrement, trap_entry=self.interrupt_controller.entry, ) diff --git a/coreblocks/frontend/fetch/fetch.py b/coreblocks/frontend/fetch/fetch.py index 70c8d1626..a53b506c8 100644 --- a/coreblocks/frontend/fetch/fetch.py +++ b/coreblocks/frontend/fetch/fetch.py @@ -1,10 +1,12 @@ from amaranth import * from amaranth.lib.data import ArrayLayout from amaranth.lib.coding import PriorityEncoder +from coreblocks.interface.keys import FetchResumeKey from transactron.lib import BasicFifo, Semaphore, ConnectTrans, logging, Pipe from transactron.lib.metrics import * from transactron.lib.simultaneous import condition from transactron.utils import MethodLayout, popcount, assign +from transactron.utils.dependencies import DependencyContext from transactron.utils.transactron_helpers import from_method_layout, make_layout from transactron import * @@ -51,12 +53,9 @@ def __init__(self, gen_params: GenParams, icache: CacheInterface, cont: Method) self.layouts = self.gen_params.get(FetchLayouts) - self.resume = Method(i=self.layouts.resume) + self.resume_from_unsafe = Method(i=self.layouts.resume) + self.resume_from_exception = Method(i=self.layouts.resume) self.stall_exception = Method() - # Fetch can be resumed to unstall from 'unsafe' instructions, and stalled because - # of exception report, both can happen at any time during normal excecution. - # ExceptionCauseRegister uses separate Transaction for it, so performace is not affected. - self.stall_exception.add_conflict(self.resume, Priority.LEFT) self.perf_fetch_utilization = TaggedCounter( "frontend.fetch.fetch_block_util", @@ -358,14 +357,31 @@ def flush(): with m.If(flush_now): m.d.sync += flushing_counter.eq(req_counter.count_next) - @def_method(m, self.resume, ready=(stalled & (flushing_counter == 0))) - def _(pc: Value, resume_from_exception: Value): - log.info(m, True, "Resuming new_pc=0x{:x} from exception={}", pc, resume_from_exception) + @def_method(m, self.resume_from_unsafe, ready=(stalled & (flushing_counter == 0))) + def _(pc: Value): + log.info(m, ~stalled_exception, "Resuming from unsafe instruction new_pc=0x{:x}", pc) + m.d.sync += current_pc.eq(pc) + # If core is stalled because of exception, effect of this call will be ignored, as + # `stalled_exception` is not changed + m.d.sync += stalled_unsafe.eq(0) + + @def_method(m, self.resume_from_exception, ready=(stalled_exception & (flushing_counter == 0))) + def _(pc: Value): + log.info(m, True, "Resuming from exception new_pc=0x{:x}", pc) + # Resume from exception has implicit priority to resume from unsafe instructions call. + # Both could happen at the same time due to resume methods being blocked. + # `resume_from_unsafe` will never overwrite `resume_from_exception` event, because there is at most one + # unsafe instruction in the core that will call resume_from_unsafe before or at the same time as + # `resume_from_exception`. + # `current_pc` is set to correct entry at a complete unstall due to method declaration order + # See https://github.com/kuznia-rdzeni/coreblocks/pull/654#issuecomment-2057478960 m.d.sync += current_pc.eq(pc) m.d.sync += stalled_unsafe.eq(0) - with m.If(resume_from_exception): - m.d.sync += stalled_exception.eq(0) + m.d.sync += stalled_exception.eq(0) + # Fetch can be resumed to unstall from 'unsafe' instructions, and stalled because + # of exception report, both can happen at any time during normal excecution. + # In case of simultaneous call, fetch will be correctly stalled, becasue separate signal is used @def_method(m, self.stall_exception) def _(): log.info(m, True, "Stalling the fetch unit because of an exception") @@ -373,6 +389,49 @@ def _(): m.d.sync += stalled_exception.eq(1) flush() + # Fetch resume verification + if self.gen_params.extra_verification: + expect_unstall_unsafe = Signal() + prev_stalled_unsafe = Signal() + unifier_ready = DependencyContext.get().get_dependency(FetchResumeKey())[0].ready + m.d.sync += prev_stalled_unsafe.eq(stalled_unsafe) + with m.FSM("running"): + with m.State("running"): + log.error(m, stalled_exception | prev_stalled_unsafe, "fetch was expected to be running") + log.error( + m, + unifier_ready, + "resume_from_unsafe unifier is ready before stall", + ) + with m.If(stalled_unsafe): + m.next = "stalled_unsafe" + with m.If(self.stall_exception.run): + m.next = "stalled_exception" + with m.State("stalled_unsafe"): + m.d.sync += expect_unstall_unsafe.eq(1) + with m.If(self.resume_from_unsafe.run): + m.d.sync += expect_unstall_unsafe.eq(0) + m.d.sync += prev_stalled_unsafe.eq(0) # it is fine to be stalled now + m.next = "running" + with m.If(self.stall_exception.run): + m.next = "stalled_exception" + log.error( + m, + self.resume_from_exception.run & ~self.stall_exception.run, + "unexpected resume_from_exception", + ) + with m.State("stalled_exception"): + with m.If(self.resume_from_unsafe.run): + log.error(m, ~expect_unstall_unsafe, "unexpected resume_from_unsafe") + m.d.sync += expect_unstall_unsafe.eq(0) + with m.If(self.resume_from_exception.run): + # unstall_form_unsafe may be skipped if excpetion was reported on unsafe instruction, + # invalid cases are verified by readiness check in running state + m.d.sync += expect_unstall_unsafe.eq(0) + m.d.sync += prev_stalled_unsafe.eq(0) # it is fine to be stalled now + with m.If(~self.stall_exception.run): + m.next = "running" + return m diff --git a/coreblocks/func_blocks/csr/csr.py b/coreblocks/func_blocks/csr/csr.py index 0d52cf947..0431914a4 100644 --- a/coreblocks/func_blocks/csr/csr.py +++ b/coreblocks/func_blocks/csr/csr.py @@ -241,11 +241,10 @@ def _(): @def_method(m, self.fetch_resume, accepted) def _(): + # This call will always execute, because there is at most one usafe instruction in the core, and it can be + # stored in unifer's Forwarder unitl resume becomes ready. # CSR instructions are never compressed, PC+4 is always next instruction - return { - "pc": instr.pc + self.gen_params.isa.ilen_bytes, - "resume_from_exception": False, - } + return {"pc": instr.pc + self.gen_params.isa.ilen_bytes} # Generate precommitting signal from precommit with Transaction().body(m): diff --git a/coreblocks/func_blocks/fu/priv.py b/coreblocks/func_blocks/fu/priv.py index 912b0fb2d..6a6e40bf3 100644 --- a/coreblocks/func_blocks/fu/priv.py +++ b/coreblocks/func_blocks/fu/priv.py @@ -139,7 +139,7 @@ def _(): with m.Else(): log.info(m, True, "Unstalling fetch from the priv unit new_pc=0x{:x}", ret_pc) # Unstall the fetch - self.fetch_resume_fifo.write(m, pc=ret_pc, resume_from_exception=0) + self.fetch_resume_fifo.write(m, pc=ret_pc) return { "rob_id": instr_rob, diff --git a/coreblocks/interface/layouts.py b/coreblocks/interface/layouts.py index 5257f62a9..3d489d4e0 100644 --- a/coreblocks/interface/layouts.py +++ b/coreblocks/interface/layouts.py @@ -455,7 +455,7 @@ def __init__(self, gen_params: GenParams): fields.predicted_taken, ) - self.resume = make_layout(("pc", gen_params.isa.xlen), ("resume_from_exception", 1)) + self.resume = make_layout(fields.pc) self.predecoded_instr = make_layout(fields.cfi_type, ("cfi_offset", signed(21)), ("unsafe", 1)) diff --git a/coreblocks/params/configurations.py b/coreblocks/params/configurations.py index b6b567c45..d02af3c3d 100644 --- a/coreblocks/params/configurations.py +++ b/coreblocks/params/configurations.py @@ -72,6 +72,8 @@ class _CoreConfigurationDataClass: Log of the size of the fetch block (in bytes). allow_partial_extensions: bool Allow partial support of extensions. + extra_verification: bool + Enables generation of additional hardware checks (asserts via logging system). Defaults to True. _implied_extensions: Extenstion Bit flag specifing enabled extenstions that are not specified by func_units_config. Used in internal tests. pma : list[PMARegion] @@ -105,6 +107,8 @@ def __post_init__(self): allow_partial_extensions: bool = False + extra_verification: bool = True + _implied_extensions: Extension = Extension(0) pma: list[PMARegion] = field(default_factory=list) diff --git a/coreblocks/params/genparams.py b/coreblocks/params/genparams.py index a13dc2528..61ada0fa0 100644 --- a/coreblocks/params/genparams.py +++ b/coreblocks/params/genparams.py @@ -76,4 +76,6 @@ def __init__(self, cfg: CoreConfiguration): self.fetch_width = 2**cfg.fetch_block_bytes_log // self.min_instr_width_bytes self.fetch_width_log = exact_log2(self.fetch_width) + self.extra_verification = cfg.extra_verification + self._toolchain_isa_str = gen_isa_string(extensions, cfg.xlen, skip_internal=True) diff --git a/test/frontend/test_fetch.py b/test/frontend/test_fetch.py index b965d8591..cf6e6624a 100644 --- a/test/frontend/test_fetch.py +++ b/test/frontend/test_fetch.py @@ -7,6 +7,7 @@ from amaranth import Elaboratable, Module from amaranth.sim import Passive +from coreblocks.interface.keys import FetchResumeKey from transactron.core import Method from transactron.lib import AdapterTrans, Adapter, BasicFifo @@ -19,6 +20,7 @@ from coreblocks.params import * from coreblocks.params.configurations import test_core_config from coreblocks.interface.layouts import ICacheLayouts, FetchLayouts +from transactron.utils.dependencies import DependencyContext class MockedICache(Elaboratable, CacheInterface): @@ -69,13 +71,11 @@ def setup(self, configure_dependency_context): fifo = BasicFifo(self.gen_params.get(FetchLayouts).raw_instr, depth=2) self.io_out = TestbenchIO(AdapterTrans(fifo.read)) self.clean_fifo = TestbenchIO(AdapterTrans(fifo.clear)) - fetch = FetchUnit(self.gen_params, self.icache, fifo.write) - self.fetch_resume = TestbenchIO(AdapterTrans(fetch.resume)) - self.fetch_stall_exception = TestbenchIO(AdapterTrans(fetch.stall_exception)) + self.fetch_resume_mock = TestbenchIO(Adapter()) + DependencyContext.get().add_dependency(FetchResumeKey(), self.fetch_resume_mock.adapter.iface) + self.fetch = SimpleTestCircuit(FetchUnit(self.gen_params, self.icache, fifo.write)) - self.m = ModuleConnector( - self.icache, fifo, self.io_out, self.clean_fifo, fetch, self.fetch_resume, self.fetch_stall_exception - ) + self.m = ModuleConnector(self.icache, fifo, self.io_out, self.clean_fifo, self.fetch) self.instr_queue = deque() self.mem = {} @@ -188,7 +188,7 @@ def fetch_out_check(self): if (instr["jumps"] and (instr["branch_taken"] != v["predicted_taken"])) or access_fault: yield from self.random_wait(5) - yield from self.fetch_stall_exception.call() + yield from self.fetch.stall_exception.call() yield from self.random_wait(5) # Empty the pipeline @@ -203,7 +203,7 @@ def fetch_out_check(self): ) + self.gen_params.fetch_block_bytes # Resume the fetch unit - while (yield from self.fetch_resume.call_try(pc=resume_pc, resume_from_exception=1)) is None: + while (yield from self.fetch.resume_from_exception.call_try(pc=resume_pc)) is None: pass def run_sim(self): diff --git a/transactron/core/method.py b/transactron/core/method.py index 85860f601..b5d573fcd 100644 --- a/transactron/core/method.py +++ b/transactron/core/method.py @@ -160,7 +160,7 @@ def proxy(self, m: "TModule", method: "Method"): Method for which this method is a proxy for. """ - @def_method(m, self) + @def_method(m, self, ready=method.ready) def _(arg): return method(m, arg)