Generalize the 2D transfer handler, and implement DO FOREVER in both engines - #4
Generalize the 2D transfer handler, and implement DO FOREVER in both engines#4elijahr wants to merge 4 commits into
Conversation
The dual-counter destination handler admitted exactly one address generation mode and hardcoded its offset register index. Every other dual-counter destination fell through to the unsupported-mode assert, so firmware programming DOR0, DOR2 or DOR3 aborted the emulator. The four registers are symmetric -- an array indexed by the mode enum -- so one generalized case replaces four copies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The operating system's idle loop executes DO FOREVER, which aborted both execution engines. The JIT gains an emitter on the existing do-immediate pattern, arming a near-maximum loop count because zero means skip; the interpreter mirrors it through the same executor. The hardware loop stack semantics ride do_exec's existing machinery: LA/LC push to the shadow stack, LF sets, and loop exit lands one past the body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ Momus review posted — verdict REQUEST_CHANGES, 1 finding
|
|
/ai-review |
![axiomantic-momus[bot]](https://avatars.githubusercontent.com/u/230757868?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR generalizes the DMA 2D-transfer handler to select DOR0-3 by AGM (correct and well-tested) and implements DO FOREVER in both engines. The DO FOREVER implementation is not 'forever': it loads LC=0x7FFFFF and relies on counted-loop termination, so the loop falls through after ~8.4M iterations and clobbers LC, which the hardware instruction does not.
Severity tally: 1 Medium.
Medium (blocking)
- BOT-A1 (
source/dsp56kEmu/dsp_ops.inl:277): DO FOREVER is emulated as a finite 8,388,607-iteration loop, not an infinite loop
Noteworthy
- The DMA DOR generalization is correct and its test genuinely enforces the AGM->DOR slot mapping by asserting both the payload location and the post-transfer DDR value.
Verdict: REQUEST_CHANGES.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.17 - 266,136 in / 52,154 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
| errNotImplemented("DO FOREVER"); | ||
| const TWord addr = absAddressExt<DoForever>(); | ||
|
|
||
| do_exec( 0x7FFFFF, addr ); |
There was a problem hiding this comment.
BOT-A1 — Medium (bug)
DO FOREVER is emulated as a finite 8,388,607-iteration loop, not an infinite loop
The 56300 'DO FOREVER' instruction does not use LC and never terminates via the loop counter; it sets LF/LA and loops indefinitely, leaving LC untouched. Both implementations instead treat it as a counted loop: dsp_ops.inl:277 is do_exec( 0x7FFFFF, addr ); and jitops.cpp:595 is DspValue lc(m_block, 0x7FFFFF, DspValue::Immediate24);. With LC=0x7FFFFF the loop exits after 8,388,607 iterations and falls through to LA+1 (dsp.cpp:518 if( reg.lc.var <= 1 )), so a program using DO FOREVER as its never-terminating main loop runs 8.4M times and then executes whatever follows, and the guest can observe LC clobbered. In the interpreter the entire loop also runs inside a single exec() call (the blocking while loop in do_exec), blocking the DSP thread for the full count. 0x7FFFFF is also an undocumented magic constant, contrary to the repo's 'explain decisions' comment convention.
| do_exec( 0x7FFFFF, addr ); | |
| Either implement true infinite-loop semantics (do not load/decrement LC, and let the existing terminate/m_terminate check bound the interpreter's blocking loop), or if a bounded count is an intentional workaround for the JIT block model, add a comment explaining why 0x7FFFFF was chosen. |
The instruction was emulated as a counted DO armed with $7FFFFF, the largest value the loop counter can hold. That is wrong in three ways the part is explicit about (DSP56300 Family Manual rev 2.0, DO FOREVER, p.13-60 and p.13-61 note 2). LC is pushed but not written: "The LC register is pushed onto the stack but is not updated by this instruction." A program is entitled to seed LC before the instruction and read it back as its own pass counter. Arming it destroyed that value. LF and FV are both set, and both are restored on the way out: ENDDO and BRKcc are specified as SSL(LF,FV) -> SR. FV existed in the register definitions and was never set by anything. The count cannot end the loop: "The LC register is never tested by the DO FOREVER instruction, and the only way of terminating the loop process is to use either the ENDDO or BRKcc instructions. LC is decremented every time PC = LA." The counter walks through 1, through 0, and wraps at 24 bits, and none of that is an exit. The interpreter gets a do_execImpl that skips the LC write and the count test when the loop is a forever loop. It can afford a genuinely unbounded loop because execInterpreter polls interrupts on every instruction inside it, and the existing m_terminate check remains the shutdown valve. The JIT gets the same LC and FV treatment plus one extra piece. A forever loop whose body fits in a single block would otherwise spin on the block's own back edge and never return, so DSP::execJit would never poll interrupts again. The loop-begin analysis now decodes the instruction that opened the loop and marks the block IsForeverLoopBody, and such a block does not close its back edge: it returns once per pass. That costs one block re-entry per iteration and is what makes the loop interruptible, which the manual calls out explicitly. The test drives both engines. It seeds LC with 3 and leaves the loop only once the recorded counter has bit 23 set, which can only happen if the instruction left LC alone and refused to retire on the count. It also checks FV inside the loop and its restoration after ENDDO, and pins the JIT to exactly one pass per exec() call.
|
BOT-A1 accepted, and it was worse than a finite loop. Pushed 1edde9a. I went to the DSP56300 Family Manual rev 2.0 (DO FOREVER, p.13-60 and p.13-61 note 2) rather than reasoning from the constant, and the manual disagrees with the old code on three separate points, not one. LC is not written at all. "The LC register is pushed onto the stack but is not updated by this instruction." Arming it with $7FFFFF destroyed a value the program is entitled to seed before the instruction and read back afterwards, which is the second half of note 2: "LC is decremented every time PC = LA so that it can be used by the programmer to keep track of the number of times the DO FOREVER loop has been executed." FV was never set. The operation is "1 -> LF; 1 -> FV", and ENDDO and BRKcc are both specified as SSL(LF,FV) -> SR. SR_FV and SRB_FV already existed in registers.h and nothing had ever written them, so ENDDO restored only LF. The count cannot end the loop. "The LC register is never tested by the DO FOREVER instruction, and the only way of terminating the loop process is to use either the ENDDO or BRKcc instructions." The counter walks through 1, through 0, and wraps at 24 bits, and none of that is an exit. I did consider declining this as a deliberate safety valve against a hung emulator, since 8,388,607 is exactly 2^23-1. The history does not support that reading: the commit message for the original change says the count was chosen "because zero means skip", i.e. it was a way to reuse the counted-DO path, not a bound anyone had reasoned about. There is also already a real valve in the interpreter, the m_terminate check inside do_exec, and it is documented as such. The interpreter can afford a genuinely unbounded loop because execInterpreter polls interrupts on every instruction inside it. The JIT needed one extra piece: a forever loop whose body fits in a single block would spin on the block's own back edge and never return, so DSP::execJit would never poll interrupts again. The loop-begin analysis now decodes the instruction that opened the loop, marks the block IsForeverLoopBody, and such a block does not close its back edge. It returns once per pass, which costs one block re-entry per iteration and is what makes the loop interruptible, exactly as the manual calls out. On the test. dsp56k_do_forever previously asserted that the loop exits to LA+1, which encoded the defect, so it is rewritten. It now seeds LC with 3 and leaves the loop only once the recorded counter has bit 23 set, which can only happen if the instruction left LC alone and refused to retire on the count. It checks FV from inside the loop and its restoration after ENDDO, and it pins the JIT to exactly one pass per exec() call. Both engines are driven: g_useJIT is a compile-time constant, so the interpreter is reached through DSP::execInterpreter() directly. Two checks that the test is not a mirage. Against the code as reviewed here it goes red on DOR FOREVER is still errNotImplemented in both engines. That is unchanged and out of scope here. |
1 participant
Two engine corrections. Based on
dsp/stack-03.DO FOREVERin both engines — the interpreter and the JIT — so the two do not disagree on a program that uses it.PR 4 of a 6-PR stack. Merge after
dsp/stack-03.