From ecc183dc663815e35a7871081d34bbf5c13ddfe0 Mon Sep 17 00:00:00 2001 From: Orual Date: Sat, 7 Mar 2026 03:44:41 -0500 Subject: [PATCH] fix: address Phase 2 code review feedback on timing and error handling - IMPORTANT 1 (AC3.10): Fix timing tests to verify exact cycle counts * test_dyadic_timing: Verify Emitted fires exactly 5 cycles after second token injection * test_monadic_timing: Verify Emitted fires exactly 4 cycles after injection * test_side_path_timing: Verify FrameAllocated fires exactly 2 cycles after injection - IMPORTANT 2: Add exhaustive match default case to _make_token_from_dest * Raises ValueError for unknown token_kind instead of silently returning None - MINOR 1: Add type annotation to _deliver method * Changed parameter from 'token' to 'token: Token' * Added Token to imports from tokens module - MINOR 2: Add warning logs in _emit_inherit for non-FrameDest slots * Log when dest_l or dest_r is not FrameDest instead of silently skipping emission - Pipeline timing fix: Restructure yield ordering to ensure correct cycle counts * Side paths: yield before handler, so events fire after complete cycle * Normal ALU: yield for EMIT before calling _do_emit_new, so Emitted fires after EMIT cycle completes --- emu/pe.py | 21 ++++++--- tests/test_pe_frames.py | 98 +++++++++++++++++++++++++++-------------- 2 files changed, 81 insertions(+), 38 deletions(-) diff --git a/emu/pe.py b/emu/pe.py index 65bf83d..a80a713 100644 --- a/emu/pe.py +++ b/emu/pe.py @@ -28,7 +28,7 @@ from emu.events import ( from emu.types import PEConfig from tokens import ( CMToken, DyadToken, FrameControlToken, - MonadToken, PELocalWriteToken, PEToken, SMToken, + MonadToken, PELocalWriteToken, PEToken, SMToken, Token, ) logger = logging.getLogger(__name__) @@ -122,13 +122,13 @@ class ProcessingElement: CMToken pipeline (IFETCH → act_id resolution → MATCH → EXECUTE → EMIT). """ if isinstance(token, FrameControlToken): - self._handle_frame_control(token) yield self.env.timeout(1) + self._handle_frame_control(token) return if isinstance(token, PELocalWriteToken): - self._handle_local_write(token) yield self.env.timeout(1) + self._handle_local_write(token) return # CMToken pipeline: IFETCH → act_id resolution → MATCH → EXECUTE → EMIT @@ -246,8 +246,9 @@ class ProcessingElement: op=inst.opcode, result=result, bool_out=bool_out, )) yield self.env.timeout(1) - self._do_emit_new(inst, result, bool_out, token.act_id, frame_id, left=left) + # EMIT cycle: complete the cycle before firing Emitted events yield self.env.timeout(1) + self._do_emit_new(inst, result, bool_out, token.act_id, frame_id, left=left) def _handle_frame_control(self, token: FrameControlToken) -> None: """Handle ALLOC and FREE operations.""" @@ -433,6 +434,9 @@ class ProcessingElement: time=self.env.now, component=self._component, token=out_token, )) self.env.process(self._deliver(self.route_table[dest_l.target_pe], out_token)) + else: + logger.warning("PE %d: frame[%d][%d] is not FrameDest: %r", + self.pe_id, frame_id, dest_base, dest_l) if inst.dest_count >= 2: dest_r = self.frames[frame_id][dest_base + 1] @@ -445,6 +449,9 @@ class ProcessingElement: self.env.process(self._deliver( self.route_table[dest_r.target_pe], out_r, )) + else: + logger.warning("PE %d: frame[%d][%d] is not FrameDest: %r", + self.pe_id, frame_id, dest_base + 1, dest_r) def _emit_change_tag( self, @@ -529,11 +536,13 @@ class ProcessingElement: target=dest.target_pe, offset=dest.offset, act_id=dest.act_id, data=data, inline=True, ) + case _: + raise ValueError(f"unknown token_kind: {dest.token_kind}") - def _deliver(self, store: simpy.Store, token) -> None: + def _deliver(self, store: simpy.Store, token: Token) -> None: """Spawn delivery process: 1 cycle delay, then put token. - MINOR FIX: Accept Token type, not just PEToken, since SMToken is also delivered. + Accepts any Token type (CMToken, SMToken, FrameControlToken) since all are delivered to stores. """ yield self.env.timeout(1) yield store.put(token) diff --git a/tests/test_pe_frames.py b/tests/test_pe_frames.py index 9f0696e..d9799f6 100644 --- a/tests/test_pe_frames.py +++ b/tests/test_pe_frames.py @@ -1073,6 +1073,11 @@ class TestPipelineTiming: """AC3.10: Pipeline timing: 5 cycles dyadic, 4 cycles monadic, 2 cycles side paths.""" def test_dyadic_timing(self): + """Verify dyadic pipeline: 5 cycles from second token injection to Emitted event. + + Pipeline stages: dequeue(1) + IFETCH(1) + MATCH(1) + EXECUTE(1) + EMIT(1) = 5 cycles. + The second token triggers the match and begins execution of the complete pipeline. + """ env = simpy.Environment() events = [] config = PEConfig(frame_count=4, on_event=events.append) @@ -1111,40 +1116,50 @@ class TestPipelineTiming: # Wire route table pe.route_table[0] = simpy.Store(env) - # Record times - token_received_time = None - emitted_time = None - - # Inject first token + # Inject first token (L operand) - will wait for partner tok1 = DyadToken(target=0, offset=100, act_id=0, data=5, port=Port.L) - inject_and_run(env, pe, tok1) + def _put1(): + yield pe.input_store.put(tok1) + env.process(_put1()) + env.run() - # Find TokenReceived event for first token + # Record time when first token was received + first_token_received_time = None for e in events: if isinstance(e, TokenReceived) and e.token == tok1: - token_received_time = e.time + first_token_received_time = e.time break - # Clear events, inject second token + # Clear events, inject second token (R operand) at a new time events.clear() + env_snapshot_time = env.now tok2 = DyadToken(target=0, offset=100, act_id=0, data=3, port=Port.R) - inject_and_run(env, pe, tok2) + def _put2(): + yield pe.input_store.put(tok2) + env.process(_put2()) + env.run() - # Find Emitted event + # Find the Emitted event for the result + emitted_time = None for e in events: if isinstance(e, Emitted): emitted_time = e.time break - # Timing: dequeue(1) + IFETCH(1) + MATCH(1) + EXECUTE(1) + EMIT(1) = 5 cycles - # But we need to account for first token's full cycle: injected at ~2, emitted at ~10 total - # The second token arrives after first finishes waiting; so: - # First token: received at time T, matched at T+2 (after second token dequeued) - # Both complete together with emission at final time - # This test just verifies we get Emitted events; precise cycle counting is complex in SimPy - assert emitted_time is not None + # Verify timing: from tok2 injection, 5 cycles should elapse to emission. + # tok2 is injected at env_snapshot_time, so emission should be at env_snapshot_time + 5. + assert first_token_received_time is not None, "First token should have TokenReceived event" + assert emitted_time is not None, "Should have Emitted event after second token" + # The delta from second token injection (env_snapshot_time) to emission should be 5 cycles + delta = emitted_time - env_snapshot_time + assert delta == 5, f"Expected 5 cycles, got {delta}" def test_monadic_timing(self): + """Verify monadic pipeline: 4 cycles from injection to Emitted event. + + Pipeline stages: dequeue(1) + IFETCH(1) + EXECUTE(1) + EMIT(1) = 4 cycles. + Monadic tokens skip the MATCH stage. + """ env = simpy.Environment() events = [] config = PEConfig(frame_count=4, on_event=events.append) @@ -1183,6 +1198,9 @@ class TestPipelineTiming: # Wire route table pe.route_table[0] = simpy.Store(env) + # Record time before injecting + injection_time = env.now + # Inject monadic token tok = MonadToken( target=0, @@ -1191,13 +1209,29 @@ class TestPipelineTiming: data=42, inline=False, ) - inject_and_run(env, pe, tok) + def _put(): + yield pe.input_store.put(tok) + env.process(_put()) + env.run() - # Should have Emitted event - emitted = [e for e in events if isinstance(e, Emitted)] - assert len(emitted) > 0 + # Find the Emitted event + emitted_time = None + for e in events: + if isinstance(e, Emitted): + emitted_time = e.time + break + + # Verify timing: 4 cycles from injection to emission + assert emitted_time is not None, "Should have Emitted event" + delta = emitted_time - injection_time + assert delta == 4, f"Expected 4 cycles for monadic token, got {delta}" def test_side_path_timing(self): + """Verify side path pipeline: 2 cycles from injection to FrameAllocated event. + + Pipeline stages: dequeue(1) + handle(1) = 2 cycles. + Side paths (FrameControlToken, PELocalWriteToken) bypass the main pipeline. + """ env = simpy.Environment() events = [] config = PEConfig(frame_count=4, on_event=events.append) @@ -1207,10 +1241,11 @@ class TestPipelineTiming: config=config, ) + # Record time before injection + injection_time = env.now + # Inject FrameControlToken(ALLOC) - side path, 2 cycles fct = FrameControlToken(target=0, act_id=0, op=FrameOp.ALLOC, payload=0) - token_received_time = None - frame_allocated_time = None def _put(): yield pe.input_store.put(fct) @@ -1218,14 +1253,13 @@ class TestPipelineTiming: env.process(_put()) env.run() + frame_allocated_time = None for e in events: - if isinstance(e, TokenReceived): - token_received_time = e.time - elif isinstance(e, FrameAllocated): + if isinstance(e, FrameAllocated): frame_allocated_time = e.time + break - # FrameAllocated should come 2 cycles after TokenReceived (dequeue 1 + handle 1) - assert token_received_time is not None - assert frame_allocated_time is not None - # In a sequential injection model, frame_allocated_time should be after dequeue+handle - assert frame_allocated_time >= token_received_time + # FrameAllocated should fire exactly 2 cycles after injection (dequeue 1 + handle 1) + assert frame_allocated_time is not None, "Should have FrameAllocated event" + delta = frame_allocated_time - injection_time + assert delta == 2, f"Expected 2 cycles for side path, got {delta}" -- 2.51.2