diff --git a/codeGenCpu6502/src/prog8/codegen/cpu6502/assignment/AssignmentAsmGen.kt b/codeGenCpu6502/src/prog8/codegen/cpu6502/assignment/AssignmentAsmGen.kt index e8324e2ee..8a166b601 100644 --- a/codeGenCpu6502/src/prog8/codegen/cpu6502/assignment/AssignmentAsmGen.kt +++ b/codeGenCpu6502/src/prog8/codegen/cpu6502/assignment/AssignmentAsmGen.kt @@ -68,10 +68,15 @@ internal class AssignmentAsmGen( // Check if we need to save A register before extracting status flags // (flag extraction functions like assignCarryFlagResult overwrite A) - val hasByteInA = registersResults.any { (ret, _) -> + // A holds the low byte of a byte/bool return in @A, and of a word return in @AX/@AY + val byteInA = registersResults.any { (ret, _) -> ret.type.isByteOrBool && ret.register.registerOrPair == RegisterOrPair.A } - if(hasByteInA && statusFlagResults.isNotEmpty()) { + val wordLowByteInA = registersResults.any { (ret, _) -> + ret.type.isWord && ret.register.registerOrPair in setOf(RegisterOrPair.AX, RegisterOrPair.AY) + } + val hasAInResult = byteInA || wordLowByteInA + if(hasAInResult && statusFlagResults.isNotEmpty()) { asmgen.out(" pha") // Save A (contains return value) before flag extraction } @@ -101,7 +106,7 @@ internal class AssignmentAsmGen( } // Restore A if we saved it for flag extraction - if(hasByteInA && statusFlagResults.isNotEmpty()) { + if(hasAInResult && statusFlagResults.isNotEmpty()) { asmgen.out(" pla") } diff --git a/codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt b/codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt index f1762521e..defee046c 100644 --- a/codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt +++ b/codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt @@ -33,9 +33,10 @@ internal class AssignmentGen(private val codeGen: IRCodeGen, private val exprGen if (extsub.returns.size == assignmentTargets.size) { // Targets and values match. Assign all the things. Skip 'void' targets. // We need to handle both FP registers (FAC1/FAC2) and regular CPU registers - // Status flag returns MUST be processed first, because their branch-based - // code needs to read CPU flags immediately after the call, before any - // LOADHR/STOREM instructions clobber them. + // Status flag returns must be read before anything that clobbers the CPU status + // bits. When the call also has non-flag register returns, those are processed + // first (they may clobber the scratch register used by the flag extraction), + // protected by an outer PUSHST/POPST so the flags stay intact for the extraction. val fpRegs = funcCall.multipleResultFpRegs.toMutableList() val cpuRegs = funcCall.multipleResultRegs.toMutableList() @@ -67,16 +68,32 @@ internal class AssignmentGen(private val codeGen: IRCodeGen, private val exprGen // flag still needs to read. Wrap every flag pair's handling in // PUSHST/POPST so each one sees the original status right after the call. val wrapStatusFlags = flagPairs.count { !isVoidTarget(it) } > 1 - flagPairs.forEach { pair -> - if (wrapStatusFlags && !isVoidTarget(pair)) { - result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) } - processPair(pair) - result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) } - } else { - processPair(pair) + fun processFlagPairs() { + flagPairs.forEach { pair -> + if (wrapStatusFlags && !isVoidTarget(pair)) { + result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) } + processPair(pair) + result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) } + } else { + processPair(pair) + } } } - otherPairs.forEach(::processPair) + + val hasStatusFlags = flagPairs.any { !isVoidTarget(it) } + val hasOtherResults = otherPairs.any { !isVoidTarget(it) } + if (hasStatusFlags && hasOtherResults) { + // The non-flag register returns are read before the flag extraction, + // so the extraction's scratch register cannot clobber them. The flag + // bits are preserved across the register loads/stores with PUSHST/POPST. + result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) } + otherPairs.forEach(::processPair) + result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) } + processFlagPairs() + } else { + processFlagPairs() + otherPairs.forEach(::processPair) + } } else { throw AssemblyError("number of values and targets don't match") } diff --git a/codeGenM68k/src/prog8/codegen/m68k/InstrControl.kt b/codeGenM68k/src/prog8/codegen/m68k/InstrControl.kt index 722bbb62c..691dd14e4 100644 --- a/codeGenM68k/src/prog8/codegen/m68k/InstrControl.kt +++ b/codeGenM68k/src/prog8/codegen/m68k/InstrControl.kt @@ -180,21 +180,18 @@ internal fun AsmGen.translateControl(insn: IRInstruction, forwardedImmediateCall // === Status flag stack ops via Scc === Opcode.PUSHST -> { - // Move CCR to D0 (low byte), then push as byte. + // Save CCR to the stack. The 68010+ memory form keeps the CCR save + // register-neutral (no data register is clobbered) and word-aligned. if(program.options.compTarget.cpu == CpuType.M68000) error("the 68000 cpu cannot save/restore the status bits using a nonprivileged instruction. This is required to implement the 'PUSHST/POPST' IR opcodes. Compile for 68010 or higher cpu or change the code such that PUSHST/POPST are no longer used (sometimes complicated rol/ror operations need it)") - invalidateD0Cache() - emitLine("move ccr, d0") - emitLine("move.b d0, -(sp)") + emitLine("move ccr, -(sp)") } Opcode.POPST -> { - // Pop byte into D0, then restore CCR + // Restore CCR from the stack (register-neutral on 68010+). if(program.options.compTarget.cpu == CpuType.M68000) error("the 68000 cpu cannot save/restore the status bits using a nonprivileged instruction. This is required to implement the 'PUSHST/POPST' IR opcodes. Compile for 68010 or higher cpu or change the code such that PUSHST/POPST are no longer used (sometimes complicated rol/ror operations need it)") - invalidateD0Cache() - emitLine("move.b (sp)+, d0") - emitLine("move d0, ccr") + emitLine("move (sp)+, ccr") } // === Flag manipulation === diff --git a/compiler/test/codegeneration/TestM68k.kt b/compiler/test/codegeneration/TestM68k.kt index 1a9b14d34..ca4fb254e 100644 --- a/compiler/test/codegeneration/TestM68k.kt +++ b/compiler/test/codegeneration/TestM68k.kt @@ -450,6 +450,52 @@ main { } } + test("status flag return is saved and register return read first on m68k") { + // Regression test: a multi-assign returning both a status flag and a register value + // used to let the flag extraction (which uses D0 as scratch for ROXL) clobber the + // register return. The register return is now read first, protected by a + // register-neutral PUSHST/POPST (the 68010+ 'move ccr,-(sp)' form). + val src = """ +main { + inline asmsub f(ubyte arg @D0) -> bool @Pc, ubyte @D0 { + %asm {{ + moveq #42, d0 + cmp.b #42, d0 + }} + } + + sub start() { + bool c + ubyte v + c, v = f(42) + } +} +""" + val result = compileText(Qemu68kTarget(), optimize = false, src, outputDir, writeAssembly = true, assemble = false) + result shouldNotBe null + val asm = result!!.compilationOptions.outputDir.resolve("${result.compilerAst.name}.asm").toFile().readText() + // scope to the start subroutine region + val start = asm.indexOf("; ---- Subroutine: p8b_main.p8s_start") + val end = asm.indexOf("; ---- Subroutine:", start + 1).let { if (it < 0) asm.length else it } + val lines = asm.substring(start, end).lines().map { it.trim() } + // register-neutral PUSHST/POPST form, no d0 round-trip + lines.count { it == "move ccr,-(sp)" } shouldBe 1 + lines.count { it == "move (sp)+,ccr" } shouldBe 1 + lines.count { it == "move ccr, d0" } shouldBe 0 + lines.count { it == "move.b d0, -(sp)" } shouldBe 0 + // the @D0 return is read (loadhr) before the flag extraction's scratch use (roxl) + val loadhrIdx = lines.indexOfFirst { it.startsWith("; loadhr.b") } + val roxlIdx = lines.indexOfFirst { it.contains("roxl") } + loadhrIdx shouldBeGreaterThan -1 + roxlIdx shouldBeGreaterThan -1 + (loadhrIdx < roxlIdx) shouldBe true + // the loadhr sits inside the PUSHST/POPST sandwich + val pushIdx = lines.indexOfFirst { it == "move ccr,-(sp)" } + val popIdx = lines.indexOfFirst { it == "move (sp)+,ccr" } + (pushIdx < loadhrIdx) shouldBe true + (loadhrIdx < popIdx) shouldBe true + } + test("struct type symbols are emitted in the header before program start") { val src = """ main { diff --git a/compiler/test/codegeneration/TestNewCodegenBugs.kt b/compiler/test/codegeneration/TestNewCodegenBugs.kt index 1e11e3386..32dfb2765 100644 --- a/compiler/test/codegeneration/TestNewCodegenBugs.kt +++ b/compiler/test/codegeneration/TestNewCodegenBugs.kt @@ -1,6 +1,7 @@ package prog8tests.codegeneration import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.ints.shouldBeGreaterThan import io.kotest.matchers.shouldBe import prog8.code.target.C64Target import prog8tests.helpers.compileText @@ -187,4 +188,107 @@ class TestNewCodegenBugs : FunSpec({ legacyMachine.assertMemory(0x343, expected[3]) } } + + test("status flag return does not clobber an A register return value") { + // Regression test: in a multi-assign returning both a status flag and a byte in A, + // the flag extraction (which uses A as scratch) used to destroy the byte return. + // The register return is now read first, protected by a PUSHST/POPST (php/plp on 6502). + val src = $$""" + %option no_sysinit + %launcher none + %address $1000 + main { + inline asmsub f(ubyte arg @A) -> bool @Pc, ubyte @A { + %asm {{ + lda #42 + cmp #42 + }} + } + + sub start() { + bool c + ubyte v + c, v = f(42) + main.v = v + main.c = c as ubyte + } + ubyte @shared v + ubyte @shared c + } + """.trimIndent() + // asm-text only, so this test does not require an external assembler + val compileResult = compileText(C64Target(), false, src, outputDir, newCodegen = true, writeAssembly = true, assemble = false)!! + val asm = compileResult.compilationOptions.outputDir.resolve(compileResult.compilerAst.name + ".asm").readText() + // scope to the start subroutine region + val start = asm.indexOf("; ---- Subroutine: p8b_main.p8s_start") + val end = asm.indexOf("; ---- Subroutine:", start + 1).let { if (it < 0) asm.length else it } + val lines = asm.substring(start, end).lines().map { it.trim() } + // the register read (loadhr) must happen before the flag extraction's lda #0 + val loadhrIdx = lines.indexOfFirst { it.startsWith("; loadhr.b") } + val flagLdaIdx = lines.indexOfFirst { it == "lda #0" } + loadhrIdx shouldBeGreaterThan -1 + flagLdaIdx shouldBeGreaterThan -1 + (loadhrIdx < flagLdaIdx) shouldBe true + // protected by a php/plp pair around the register read + lines.count { it == "php" } shouldBe 1 + lines.count { it == "plp" } shouldBe 1 + } + + test("legacy 6502 word @AX/@AY return saves A across status flag extraction") { + val src = $$""" + %option no_sysinit + main { + asmsub fAX(ubyte a @A) -> bool @Pc, uword @AX { + %asm {{ + lda #42 + ldx #1 + cmp #42 + rts + }} + } + asmsub fAY(ubyte a @A) -> bool @Pc, uword @AY { + %asm {{ + lda #42 + ldy #1 + cmp #42 + rts + }} + } + sub start() { + bool c + uword w + c, w = fAX(1) + main.w = w + c, w = fAY(1) + main.w = w + } + uword @shared w + } + """.trimIndent() + // asm-text only, so this test does not require an external assembler + val compileResult = compileText(C64Target(), false, src, outputDir, newCodegen = false, writeAssembly = true, assemble = false)!! + val asm = compileResult.compilationOptions.outputDir.resolve(compileResult.compilerAst.name + ".asm").readText() + // scope to the start subroutine region (legacy asm delimits top-level subroutines with a "\t.proc" label at column 0) + val rawLines = asm.lines() + val from = rawLines.indexOfFirst { it.startsWith("p8s_start\t.proc") } + val to = rawLines.drop(from + 1).indexOfFirst { it.endsWith(".proc") && !it.startsWith(" ") && !it.startsWith("\t") } + .let { if (it < 0) rawLines.size else from + 1 + it } + val lines = rawLines.subList(from, to).map { it.trim() } + // the flag extraction clobbers A, which holds the low byte of the word result, + // so A must be saved with pha and restored with pla before the pair is stored + lines.count { it == "pha" } shouldBe 2 + lines.count { it == "pla" } shouldBe 2 + val phaIdx = lines.indexOfFirst { it == "pha" } + val flagIdx = lines.indexOfFirst { it == "lda #0" } + val plaIdx = lines.indexOfFirst { it == "pla" } + // the first store of the word variable's low byte + val staIdx = lines.indexOfFirst { it.startsWith("sta ") && it.endsWith("p8v_w") } + phaIdx shouldBeGreaterThan -1 + flagIdx shouldBeGreaterThan -1 + plaIdx shouldBeGreaterThan -1 + staIdx shouldBeGreaterThan -1 + (phaIdx < flagIdx) shouldBe true + (flagIdx < plaIdx) shouldBe true + (plaIdx < staIdx) shouldBe true + } }) diff --git a/compiler/test/vm/TestCompilerVirtual.kt b/compiler/test/vm/TestCompilerVirtual.kt index 411229d22..4ce573fb5 100644 --- a/compiler/test/vm/TestCompilerVirtual.kt +++ b/compiler/test/vm/TestCompilerVirtual.kt @@ -1753,6 +1753,43 @@ main { irContent shouldContain "bstvc" } + test("asmsub multi-assign with a status flag and a register return orders the register read first") { + // The register return must be read before the flag extraction (which uses a + // scratch hardware register), protected by an outer PUSHST/POPST. + val src = """ +%option no_sysinit +main { + sub start() { + ubyte v + bool c + v, c = test() + main.r1 = v + main.b1 = c + } + asmsub test() -> ubyte @A, bool @Pc { + %asm {{ + lda #42 + cmp #42 + rts + }} + } + ubyte @shared r1 + bool @shared b1 +}""" + val result = compileText(VMTarget(), optimize=false, src, outputDir, writeAssembly = true)!! + val virtfile = result.compilationOptions.outputDir.resolve(result.compilerAst.name + ".p8ir") + val lines = virtfile.readText().lines().map { it.trim() } + // one outer PUSHST/POPST pair (only one flag, so no per-flag wrapping) + lines.count { it == "pushst" } shouldBe 1 + lines.count { it == "popst" } shouldBe 1 + // the register read (loadhr) must come before the flag extraction (roxl) + val loadhrIdx = lines.indexOfFirst { it.startsWith("loadhr") } + val roxlIdx = lines.indexOfFirst { it.startsWith("roxl") } + loadhrIdx shouldBeGreaterThan -1 + roxlIdx shouldBeGreaterThan -1 + (loadhrIdx < roxlIdx) shouldBe true + } + test("asmsub multi-assign with multiple status flags in one call saves the status around each flag") { val src = """ %option no_sysinit @@ -1783,10 +1820,11 @@ main { val result = compileText(VMTarget(), optimize=false, src, outputDir, writeAssembly = true)!! val virtfile = result.compilationOptions.outputDir.resolve(result.compilerAst.name + ".p8ir") val irContent = virtfile.readText() - // each of the three status flags needs its own PUSHST/POPST pair, because the + // 1 outer PUSHST/POPST pair protects the register return read plus the flags across it, + // and each of the three status flags needs its own PUSHST/POPST pair, because the // branch-based extraction of one flag clobbers the status for the following flags - irContent.lines().count { it.trim() == "pushst" } shouldBe 3 - irContent.lines().count { it.trim() == "popst" } shouldBe 3 + irContent.lines().count { it.trim() == "pushst" } shouldBe 4 + irContent.lines().count { it.trim() == "popst" } shouldBe 4 } test("union zero-init via memory, write byte and read overlapping word") { diff --git a/docs/source/todo.rst b/docs/source/todo.rst index d17d94322..372170a25 100644 --- a/docs/source/todo.rst +++ b/docs/source/todo.rst @@ -14,7 +14,6 @@ Future Things and Ideas - support typed pointer arrays as struct fields, curretly requires untyped pointers arrays. - allow aliasing a whole enum (``alias myEnum = MyEnum``) so that ``myEnum::Member`` works just like ``MyEnum::Member``; currently only aliasing individual enum members is supported. See ideas/alias-enums.md for the plan. - evaluate the feature suggestions in ideas/c16-banked-data.md (cx16 banked data library). -- Status flag extraction clobbers a register return value: in a multi-assign where one call returns both a status flag and a regular register value (e.g. ``-> bool @Pc, ubyte @A`` on new6502, or ``-> bool @Pc, ubyte @D0`` on m68k), the status flag extraction destroys the register return before it is read. The extraction uses a scratch hardware register (``A`` on 6502, ``D0`` on m68k) that can be the same register holding the return value. The old 6502 codegen avoids this with ``pha``/``pla``. See ideas/status-flag-register-clobber.md for the design (reorder the register returns before the flag extraction, protected by register-neutral ``PUSHST``/``POPST``). Won't do's or deferred ^^^^^^^^^^^^^^^^^^^^^^ diff --git a/ideas/status-flag-register-clobber.md b/ideas/status-flag-register-clobber.md deleted file mode 100644 index 8888c7dd9..000000000 --- a/ideas/status-flag-register-clobber.md +++ /dev/null @@ -1,267 +0,0 @@ -# Status flag extraction clobbers a register return value - -**Status: deferred / design only, not implemented.** Listed in -`docs/source/todo.rst`. - -Note: the *multiple* status flag returns case is already fixed (each flag -extraction is wrapped in `PUSHST`/`POPST` in `AssignmentGen`). This document -covers the separate case where a status flag return is combined with a regular -register return that lives in the extraction's scratch register. - -## 1. Problem - -In a multi-assignment where one call returns both a CPU status flag and a -regular register value, the flag extraction can destroy the register return -before it is read. Example: - -```prog8 -main { - inline asmsub f(ubyte arg @A) -> bool @Pc, ubyte @A { - %asm {{ - lda #42 - cmp #42 - }} - } - - sub start() { - bool c - ubyte v - c, v = f(42) ; v is NOT 42, it is the clobbered scratch value - } -} -``` - -This is reachable in practice, e.g. the f256 syslib `GETIN() -> bool @Pc, ubyte @A` -(`examples/customtarget/libraries/f256/syslib.p8:39`). - -The bug is *separate* from the "multiple status flag returns clobber each other" -issue (which was fixed by wrapping each flag extraction in `PUSHST`/`POPST` in -`AssignmentGen`). That fix protects the status bits; it does not protect a -hardware register that holds a non-flag return value. - -### When it triggers - -The bug needs a **multi-assign** from an `asmsub`/`extsub` (only those can -declare status flag returns) whose returned **status flag target is not `void`** -and whose **register return target is not `void`**, where the register value -lives in the extraction's scratch register: - -- **new6502 / 6502**: scratch is `A`. - - `@A` (byte/bool) return: broken with *any* flag (`@Pc`/`@Pz`/`@Pn`/`@Pv`), - because every flag extraction loads a `0`/`1` immediate through `A`. - - `@AX`/`@AY` (word) return: broken too, since `A` is its low byte. - - `@X`/`@Y`-only returns: safe. -- **m68k**: scratch is `D0`, but only the `@Pc` extraction uses it (the `ROXL` - round-trip). `@Pz`/`@Pn`/`@Pv` lower to a branch plus a direct memory store. - - `@D0` + `@Pc`: broken. - - `@D0` + `@Pz`/`@Pn`/`@Pv`: safe. - - `@D1` and higher: safe. -- **virtual (VM)**: never (extraction only uses IR virtual registers). - -The return order does not matter (flags are always extracted before the register -returns), and it is irrelevant where the value is stored (variable, register, or -memory). - -## 2. Root cause - -The IR generator (`codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt`) -processes a multi-assign in this order: - -1. **status flag returns first** (they must read the live CPU status right after - the call), then -2. **non-flag register returns** via `LOADHR` (hardware register -> IR virtual - register). - -The status flag extraction sequence uses a **scratch hardware register**: - -| target | extraction scratch | where | -| --- | --- | --- | -| new6502 | `A` | `lda #0` before `ROXL`, and the `lda`-based stores | -| m68k | `D0` | `ROXL` is lowered as `move.b regfile,d0; roxl.b #1,d0; move.b d0,regfile` (`codeGenM68k/.../InstrBitwise.kt:357`) | -| virtual (VM) | none (operates on IR virtual registers only) | unaffected | - -If a non-flag return lives in that same scratch register, the extraction -destroys it before `LOADHR` reads it. - -### Evidence (new6502, `-target c64 -newcodegen`) - -``` -; call p8b_main.p8s_f():r1.b@Pc,r2.b@s0 -; inlined: p8b_main.p8s_f -lda #42 -cmp #42 -; end inlined -; load.b r1.b,#0.b -lda #0 <-- clobbers A -sta p8_regfile+0 -; roxl.b r1.b -rol p8_regfile+0 -; storem.b r1.b,[c] -lda p8_regfile+0 -sta c -; loadhr.b r2.b,s0.b <-- reads the clobbered A, not 42 -sta p8_regfile+1 -``` - -### Evidence (m68k, `-target qemu68k`) - -``` -bsr p8b_main.p8s_f -; load.b r1.b,#0.b -clr.b p8_regfile+0 -; roxl.b r1.b -move.b p8_regfile+0,d0 <-- clobbers d0 (which held the @D0 return) -roxl.b #1,d0 -; storem.b r1.b,[c] -move.b d0,p8v_c -; loadhr.b r2.b,s10.b -; storem.b r2.b,[v] -move.b d0,p8v_v <-- stores the clobbered d0 -``` - -### Legacy 6502 reference - -The old 6502 codegen handles the byte/bool-in-`A` case with `pha`/`pla`: - -- `codeGenCpu6502/.../assignment/AssignmentAsmGen.kt:69-76` - if a byte/bool - return is in `A` and there are status flags, emit `pha` before the flag - extraction. -- `:103-106` - `pla` before storing the register results. - -Note that `hasByteInA` (`:69`) only checks `ret.type.isByteOrBool && -registerOrPair == A`, so legacy does **not** cover a word return in `@AX`/`@AY` -and shares that variant of the bug (the low byte in `A` is clobbered). The -proposed design below covers all of these cases. - -## 3. Design - -### 3.1 Key enabler: register-neutral PUSHST/POPST on m68k - -The recommended fix relies on `PUSHST`/`POPST` not clobbering any data register. -On new6502 they are already neutral (`php`/`plp`). On m68k the current -implementation goes through `d0`: - -``` -move ccr,d0 -move.b d0,-(sp) ; PUSHST -move.b (sp)+,d0 -move d0,ccr ; POPST -``` - -The 68010 `MOVE CCR` instruction also supports a **memory** operand, so the -round-trip is unnecessary: - -``` -move ccr,-(sp) ; PUSHST (encoding 42E7) -move (sp)+,ccr ; POPST (encoding 44DF) -``` - -This saves/restores the CCR without touching any data register, and keeps the -stack word-aligned (the current byte push does not, which the m68k coding -conventions warn against). - -Verified by assembling with `vasmm68k_mot -m68020` and by executing a CCR -round-trip under `qemu-system-m68k -M virt -cpu m68020` (a program that sets -C=1, pushes the CCR, clears the flags, pops the CCR back, then takes `if_cs` -prints the expected result). - -### 3.2 IR generator change (`AssignmentGen.translate`) - -Only when a call has **at least one status-flag return AND at least one -non-flag result** (register or float), reorder so the register/float returns are -materialized first, protected by a single `PUSHST`/`POPST`: - -``` -CALL -PUSHST ; save post-call status S0 -[otherPairs: LOADHR + store] ; read return registers before extraction; - ; may clobber flags and scratch registers -POPST ; restore S0 -[flagPairs: per-flag PUSHST/extract+store/POPST when >1 flag] -``` - -- Register/float values are safely in IR virtual registers before the flag - extraction clobbers the scratch register. -- The float case (`LOADHFACZERO`/`LOADHFACONE`, which lowers to a `jsr` on 6502) - is covered by the same outer save/restore. -- The existing per-flag `PUSHST`/`POPST` sandwich (for >1 status flag) is kept - unchanged; this change is orthogonal. -- Flags-only and registers-only multi-assigns keep the current codegen (no - extra instructions). - -Pseudo-code in the extsub branch: - -```kotlin -val hasStatusFlags = flagPairs.any { !isVoidTarget(it) } -val hasOtherResults = otherPairs.any { !isVoidTarget(it) } -if (hasStatusFlags && hasOtherResults) { - result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) } - otherPairs.forEach(::processPair) - result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) } - flagPairs.forEach { pair -> wrap-if-multi(pair) } -} else { - flagPairs.forEach { pair -> wrap-if-multi(pair) } - otherPairs.forEach(::processPair) -} -``` - -The "status flags MUST be processed first" comment in `AssignmentGen.kt:36` is -then no longer accurate; it becomes "status flags must be read before anything -that clobbers them, which is guaranteed by the surrounding PUSHST/POPST". - -### 3.3 m68k backend change (`codeGenM68k/.../InstrControl.kt:180-200`) - -Change `PUSHST`/`POPST` to the memory form and drop the `d0` round-trip (and the -now-unnecessary `invalidateD0Cache()` calls). The 68000 guard stays: the 68000 -cannot save the CCR with a nonprivileged instruction. - -No changes are needed in new6502, the VM, or the IR file format. - -## 4. Alternatives considered - -- **New IR opcodes `PUSHHR`/`POPHR`** (save/restore a hardware register slot) - around the flag extraction. Works, and keeps `PUSHST`/`POPST` semantics - unchanged, but requires IR enum changes, `IRTextCodec`/reader support, - peephole side-effect sets, VM support, and implementations in both backends. - More plumbing for the same result. -- **d0-preserving `PUSHST`** (push `d0` plus the CCR, restore both). Legal on - all m68k CPUs, but leaves `d0` clobbered *between* `PUSHST` and `POPST`, so a - `LOADHR d0` inside the wrap still reads garbage. Rejected. -- **6502-only reorder without save/restore.** On 6502 `LOADHR` (`sta`/`stx`/ - `sty`) is flag-neutral, so the reorder alone would work there, but the stores - and the m68k `move`-based `LOADHR` are not, so it does not generalize. - -## 5. Edge cases - -- `void` targets: excluded from both the wrap condition and the per-pair - handling, as today. -- Mixed status flag + `@A`/`@D0` + float returns: covered by the same outer - save/restore. -- `amiga500` (68000): remains a hard codegen error, now triggered only for the - flags-plus-register combination. -- Portability note: `move ccr,-(sp)` / `move (sp)+,ccr` are 68010+ instructions. - The only non-68000 m68k target is `qemu68k` (68020), so this is safe today. If - a 68010 target is ever added it still works; a 68000 target cannot use it at - all (already an error). - -## 6. Test plan - -- **ksim65 regression** (`compiler/test/codegeneration/TestNewCodegenBugs.kt`): - `-> bool @Pc, ubyte @A` returning 42, assert the byte survives; run with - `newCodegen=true` and with the legacy codegen as the reference, for - `optimize` false and true. -- **m68k**: a `TestM68k`-style test asserting the `move ccr,-(sp)` sandwich and - that no `d0` round-trip remains around the extraction; if feasible, a - `qemu68k` execution check. -- **VM**: an IR-text test asserting the reorder/save appears (asmsub bodies - cannot be executed by the VM). - -## 7. Affected files - -- `codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt` - reorder - + outer `PUSHST`/`POPST`. -- `codeGenM68k/src/prog8/codegen/m68k/InstrControl.kt` - register-neutral - `PUSHST`/`POPST`. -- `compiler/test/codegeneration/TestNewCodegenBugs.kt`, - `compiler/test/codegeneration/TestM68k.kt`, - `compiler/test/vm/TestCompilerVirtual.kt` - tests.