cpu: fix cycle count for conditional long branches - #14
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts M6809 emulation to correct cycle counting for conditional long branches and fixes RTI register restore behavior by reading the stacked CC first.
Changes:
- Update RTI to pull CC first so the stacked E flag determines which registers get restored.
- Refine long-branch cycle accounting to avoid adding the “taken” cycle for unconditional LBRA/LBRN.
- Bump the vcpkg submodule reference.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| vcpkg | Updates the vcpkg submodule commit pointer. |
| src/m6809.h | Fixes RTI restore logic (E flag source) and adjusts long-branch cycle counting rules (LBRA/LBRN vs conditional). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // First pull CC from the stack to get the E flag | ||
| cpu.registers.CC = cpu.Pull8(cpu.registers.SP); | ||
| cycles += 1; | ||
| // Now check E flag in the pulled CC to determine if we restore all registers | ||
| // 0xfe = all except CC (already pulled), 0x80 = just PC | ||
| uint8_t register_mask = (cpu.registers.flags.E) ? (uint8_t)0xfe : (uint8_t)0x80; | ||
| op_pull<reg_sp, reg_usp>()(cpu, register_mask, cycles); |
There was a problem hiding this comment.
The new cycles += 1; is a hard-coded timing adjustment that isn’t explained relative to how op_pull(...) accounts for timing. To prevent future timing regressions, please document why this specific increment is correct (e.g., reference the RTI timing model used in this emulator) and how it composes with op_pull’s per-byte/per-register cycle accounting.
| // Unconditional=true skips the extra cycle for LBRA/LBRN | ||
| template <typename Test, typename T, bool Negate=false, bool Unconditional=false> |
There was a problem hiding this comment.
Introducing an additional boolean template parameter (Unconditional) makes call sites with multiple bools harder to read and easier to misuse (e.g., ..., false, true requires readers to remember the parameter order). Consider replacing the boolean with a small tag type/enum policy, or adding dedicated aliases/wrappers for LBRA/LBRN so the call site communicates intent without positional booleans.
| template <typename Fn, bool negate=false, bool unconditional=false> | ||
| using op_bra_long = op_bra<Fn, int16_t, negate, unconditional>; |
There was a problem hiding this comment.
Introducing an additional boolean template parameter (Unconditional) makes call sites with multiple bools harder to read and easier to misuse (e.g., ..., false, true requires readers to remember the parameter order). Consider replacing the boolean with a small tag type/enum policy, or adding dedicated aliases/wrappers for LBRA/LBRN so the call site communicates intent without positional booleans.
| using op_lbra_inherent = opcode_count<op_bra_long<op_bra_always, false, true>, RegisterPC, inherent, NoFlags16, 5>; | ||
| using op_lbrn_inherent = opcode_count<op_bra_long<op_bra_always, true, true>, RegisterPC, inherent, NoFlags16, 5>; |
There was a problem hiding this comment.
Introducing an additional boolean template parameter (Unconditional) makes call sites with multiple bools harder to read and easier to misuse (e.g., ..., false, true requires readers to remember the parameter order). Consider replacing the boolean with a small tag type/enum policy, or adding dedicated aliases/wrappers for LBRA/LBRN so the call site communicates intent without positional booleans.
| using op_lbra_inherent = opcode_count<op_bra_long<op_bra_always, false, true>, RegisterPC, inherent, NoFlags16, 5>; | |
| using op_lbrn_inherent = opcode_count<op_bra_long<op_bra_always, true, true>, RegisterPC, inherent, NoFlags16, 5>; | |
| // Long branch operation aliases to avoid positional boolean template arguments. | |
| using op_bra_long_always = op_bra_long<op_bra_always, false, true>; // LBRA: long branch always | |
| using op_bra_long_never = op_bra_long<op_bra_always, true, true>; // LBRN: long branch never | |
| using op_lbra_inherent = opcode_count<op_bra_long_always, RegisterPC, inherent, NoFlags16, 5>; | |
| using op_lbrn_inherent = opcode_count<op_bra_long_never, RegisterPC, inherent, NoFlags16, 5>; |
No description provided.