From cc02760c3955695c4710cd25ce5ee64a5a126884 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Urba=C5=84czyk?= Date: Mon, 1 Apr 2024 11:13:13 +0100 Subject: [PATCH] Fix instruction cache refilling bug (#636) --- coreblocks/cache/icache.py | 77 ++++++++++++++++++++------------------ test/cache/test_icache.py | 24 ++++++++++++ transactron/lib/fifo.py | 8 ++++ transactron/lib/reqres.py | 9 ++++- 4 files changed, 79 insertions(+), 39 deletions(-) diff --git a/coreblocks/cache/icache.py b/coreblocks/cache/icache.py index bcfbd37cc..0b60cf37c 100644 --- a/coreblocks/cache/icache.py +++ b/coreblocks/cache/icache.py @@ -142,14 +142,13 @@ def elaborate(self, platform): ] m.submodules.mem = self.mem = ICacheMemory(self.params) - m.submodules.req_fifo = self.req_fifo = FIFO(layout=self.addr_layout, depth=2) - m.submodules.res_fwd = self.res_fwd = Forwarder(layout=self.layouts.accept_res) + m.submodules.req_zipper = req_zipper = ArgumentsToResultsZipper(self.addr_layout, self.layouts.accept_res) # State machine logic needs_refill = Signal() refill_finish = Signal() - refill_finish_last = Signal() refill_error = Signal() + refill_error_saved = Signal() flush_start = Signal() flush_finish = Signal() @@ -158,6 +157,7 @@ def elaborate(self, platform): self.perf_flushes.incr(m, cond=flush_finish) with m.FSM(reset="FLUSH") as fsm: + with m.State("FLUSH"): with m.If(flush_finish): m.next = "LOOKUP" @@ -180,35 +180,44 @@ def elaborate(self, platform): m.d.sync += way_selector.eq(way_selector.rotate_left(1)) # Fast path - read requests - request_valid = self.req_fifo.read.ready - request_addr = Signal(self.addr_layout) + mem_read_addr = Signal(self.addr_layout) + prev_mem_read_addr = Signal(self.addr_layout) + m.d.comb += assign(mem_read_addr, prev_mem_read_addr) - tag_hit = [tag_data.valid & (tag_data.tag == request_addr.tag) for tag_data in self.mem.tag_rd_data] - tag_hit_any = reduce(operator.or_, tag_hit) + mem_read_output_valid = Signal() + with Transaction(name="MemRead").body( + m, request=fsm.ongoing("LOOKUP") & (mem_read_output_valid | refill_error_saved) + ): + req_addr = req_zipper.peek_arg(m) - mem_out = Signal(self.params.fetch_block_bytes * 8) - for i in OneHotSwitchDynamic(m, Cat(tag_hit)): - m.d.comb += mem_out.eq(self.mem.data_rd_data[i]) + tag_hit = [tag_data.valid & (tag_data.tag == req_addr.tag) for tag_data in self.mem.tag_rd_data] + tag_hit_any = reduce(operator.or_, tag_hit) - refill_error_saved = Signal() - m.d.comb += needs_refill.eq(request_valid & ~tag_hit_any & ~refill_error_saved) + with m.If(tag_hit_any | refill_error_saved): + self.perf_hits.incr(m, cond=tag_hit_any) + mem_out = Signal(self.params.fetch_block_bytes * 8) + for i in OneHotSwitchDynamic(m, Cat(tag_hit)): + m.d.av_comb += mem_out.eq(self.mem.data_rd_data[i]) + + req_zipper.write_results(m, fetch_block=mem_out, error=refill_error_saved) + m.d.sync += refill_error_saved.eq(0) + m.d.sync += mem_read_output_valid.eq(0) + with m.Else(): + self.perf_misses.incr(m) - with Transaction().body(m, request=request_valid & fsm.ongoing("LOOKUP") & (tag_hit_any | refill_error_saved)): - self.perf_errors.incr(m, cond=refill_error_saved) - self.perf_misses.incr(m, cond=refill_finish_last) - self.perf_hits.incr(m, cond=~refill_finish_last) + m.d.comb += needs_refill.eq(1) - self.res_fwd.write(m, fetch_block=mem_out, error=refill_error_saved) - m.d.sync += refill_error_saved.eq(0) + # Align to the beginning of the cache line + aligned_addr = self.serialize_addr(req_addr) & ~((1 << self.params.offset_bits) - 1) + log.debug(m, True, "Refilling line 0x{:x}", aligned_addr) + self.refiller.start_refill(m, addr=aligned_addr) @def_method(m, self.accept_res) def _(): - self.req_fifo.read(m) self.req_latency.stop(m) - return self.res_fwd.read(m) - mem_read_addr = Signal(self.addr_layout) - m.d.comb += assign(mem_read_addr, request_addr) + output = req_zipper.read(m) + return output.results @def_method(m, self.issue_req, ready=accepting_requests) def _(addr: Value) -> None: @@ -216,11 +225,11 @@ def _(addr: Value) -> None: self.req_latency.start(m) deserialized = self.deserialize_addr(addr) - # Forward read address only if the method is called m.d.comb += assign(mem_read_addr, deserialized) - m.d.sync += assign(request_addr, deserialized) + m.d.sync += assign(prev_mem_read_addr, deserialized) + req_zipper.write_args(m, deserialized) - self.req_fifo.write(m, deserialized) + m.d.sync += mem_read_output_valid.eq(1) m.d.comb += [ self.mem.tag_rd_index.eq(mem_read_addr.index), @@ -242,18 +251,12 @@ def _() -> None: m.d.comb += flush_finish.eq(flush_index == self.params.num_of_sets - 1) # Slow path - data refilling - with Transaction().body(m, request=fsm.ongoing("LOOKUP") & needs_refill): - # Align to the beginning of the cache line - aligned_addr = self.serialize_addr(request_addr) & ~((1 << self.params.offset_bits) - 1) - log.debug(m, True, "Refilling line 0x{:x}", aligned_addr) - self.refiller.start_refill(m, addr=aligned_addr) - - m.d.sync += refill_finish_last.eq(0) - with Transaction().body(m): ret = self.refiller.accept_refill(m) deserialized = self.deserialize_addr(ret.addr) + self.perf_errors.incr(m, cond=ret.error) + m.d.top_comb += [ self.mem.data_wr_addr.index.eq(deserialized["index"]), self.mem.data_wr_addr.offset.eq(deserialized["offset"]), @@ -262,9 +265,9 @@ def _() -> None: m.d.comb += self.mem.data_wr_en.eq(1) m.d.comb += refill_finish.eq(ret.last) - m.d.sync += refill_finish_last.eq(1) m.d.comb += refill_error.eq(ret.error) - m.d.sync += refill_error_saved.eq(ret.error) + with m.If(ret.error): + m.d.sync += refill_error_saved.eq(1) with m.If(fsm.ongoing("FLUSH")): m.d.comb += [ @@ -277,9 +280,9 @@ def _() -> None: with m.Else(): m.d.comb += [ self.mem.way_wr_en.eq(way_selector), - self.mem.tag_wr_index.eq(request_addr.index), + self.mem.tag_wr_index.eq(mem_read_addr.index), self.mem.tag_wr_data.valid.eq(~refill_error), - self.mem.tag_wr_data.tag.eq(request_addr.tag), + self.mem.tag_wr_data.tag.eq(mem_read_addr.tag), self.mem.tag_wr_en.eq(refill_finish), ] diff --git a/test/cache/test_icache.py b/test/cache/test_icache.py index 43f800a5e..f53cff894 100644 --- a/test/cache/test_icache.py +++ b/test/cache/test_icache.py @@ -715,6 +715,30 @@ def cache_process(): yield from self.expect_resp(wait=True) yield yield from self.m.accept_res.disable() + yield + + # The second request will cause an error + yield from self.send_req(addr=0x00021004) + yield from self.send_req(addr=0x00030000) + + yield from self.tick(10) + + # Accept the first response + yield from self.m.accept_res.enable() + yield from self.expect_resp(wait=True) + yield + + # Wait before accepting the second response + yield from self.m.accept_res.disable() + yield from self.tick(10) + yield from self.m.accept_res.enable() + yield from self.expect_resp(wait=True) + + yield + + # This request should not cause an error + yield from self.send_req(addr=0x00011000) + yield from self.expect_resp(wait=True) with self.run_simulation(self.m) as sim: sim.add_sync_process(cache_process) diff --git a/transactron/lib/fifo.py b/transactron/lib/fifo.py index 92ac0f7bb..24cacfadc 100644 --- a/transactron/lib/fifo.py +++ b/transactron/lib/fifo.py @@ -13,6 +13,9 @@ class BasicFifo(Elaboratable): read: Method Reads from the FIFO. Accepts an empty argument, returns a structure. Ready only if the FIFO is not empty. + peek: Method + Returns the element at the front (but not delete). Ready only if the FIFO + is not empty. The method is nonexclusive. write: Method Writes to the FIFO. Accepts a structure, returns empty result. Ready only if the FIFO is not full. @@ -40,6 +43,7 @@ def __init__(self, layout: MethodLayout, depth: int, *, src_loc: int | SrcLoc = src_loc = get_src_loc(src_loc) self.read = Method(o=self.layout, src_loc=src_loc) + self.peek = Method(o=self.layout, nonexclusive=True, src_loc=src_loc) self.write = Method(i=self.layout, src_loc=src_loc) self.clear = Method(src_loc=src_loc) self.head = Signal(from_method_layout(layout)) @@ -93,6 +97,10 @@ def _() -> ValueLike: m.d.sync += self.read_idx.eq(next_read_idx) return self.head + @def_method(m, self.peek, self.read_ready) + def _() -> ValueLike: + return self.head + @def_method(m, self.clear) def _() -> None: m.d.sync += self.read_idx.eq(0) diff --git a/transactron/lib/reqres.py b/transactron/lib/reqres.py index f9aeb6e06..a3f6e2908 100644 --- a/transactron/lib/reqres.py +++ b/transactron/lib/reqres.py @@ -1,7 +1,7 @@ from amaranth import * from ..core import * from ..utils import SrcLoc, get_src_loc, MethodLayout -from .connectors import Forwarder, FIFO +from .connectors import Forwarder from transactron.lib import BasicFifo from amaranth.utils import * @@ -39,6 +39,8 @@ class ArgumentsToResultsZipper(Elaboratable): Attributes ---------- + peek_arg: Method + A nonexclusive method to read (but not delete) the head of the arg queue. write_args: Method Method to write arguments with `args_layout` format to 2-FIFO. write_results: Method @@ -65,6 +67,7 @@ def __init__(self, args_layout: MethodLayout, results_layout: MethodLayout, src_ self.args_layout = args_layout self.output_layout = [("args", self.args_layout), ("results", results_layout)] + self.peek_arg = Method(o=self.args_layout, nonexclusive=True, src_loc=self.src_loc) self.write_args = Method(i=self.args_layout, src_loc=self.src_loc) self.write_results = Method(i=self.results_layout, src_loc=self.src_loc) self.read = Method(o=self.output_layout, src_loc=self.src_loc) @@ -72,7 +75,7 @@ def __init__(self, args_layout: MethodLayout, results_layout: MethodLayout, src_ def elaborate(self, platform): m = TModule() - fifo = FIFO(self.args_layout, depth=2, src_loc=self.src_loc) + fifo = BasicFifo(self.args_layout, depth=2, src_loc=self.src_loc) forwarder = Forwarder(self.results_layout, src_loc=self.src_loc) m.submodules.fifo = fifo @@ -92,6 +95,8 @@ def _(): results = forwarder.read(m) return {"args": args, "results": results} + self.peek_arg.proxy(m, fifo.peek) + return m