Fix instruction cache refilling bug (#636)

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,47 +180,56 @@ 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:
             self.perf_loads.incr(m)
             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),
             ]
 

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)

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)

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,14 +67,15 @@ 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)
 
     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