diff --git a/codeGenIntermediate/src/prog8/codegen/intermediate/StackFrameLayout.kt b/codeGenIntermediate/src/prog8/codegen/intermediate/StackFrameLayout.kt index f40df1b98..f30a33d8c 100644 --- a/codeGenIntermediate/src/prog8/codegen/intermediate/StackFrameLayout.kt +++ b/codeGenIntermediate/src/prog8/codegen/intermediate/StackFrameLayout.kt @@ -1,9 +1,6 @@ package prog8.codegen.intermediate -import prog8.code.core.BaseDataType -import prog8.code.core.DataType -import prog8.code.core.IErrorReporter -import prog8.code.core.ZeropageWish +import prog8.code.core.* import prog8.intermediate.* /** @@ -285,8 +282,9 @@ class StackFrameLayout(private val program: IRProgram, private val errors: IErro val elemDt = isFrameableParameter(v, name, addressTaken, asmTexts) ?: return static("parameter '$name' cannot be moved into a frame") if (v.length != null) return static("parameter '$name' is an array") // a parameter referenced from outside the subroutine (defer handler, inline assembly) - // keeps its static cell for those external references and gets an entry-time copy-in - // from its incoming slot (§6.1); its address may not be taken (that stays a hard error) + // keeps its static cell for ALL references (including the body's own uses) and gets + // an entry-time copy-in from its incoming slot (§6.1); its address may not be taken + // (that stays a hard error) val refs = referencedIn[name] if ((refs != null && refs.any { it != sub.label }) || isLabelReferencedInAsm(name, asmTexts)) copyInParams += name @@ -393,13 +391,18 @@ class StackFrameLayout(private val program: IRProgram, private val errors: IErro } // rewrite references: Symbol base -> FrameSlot base (parameters additionally get the - // right-justification displacement of their padded slot) + // right-justification displacement of their padded slot). + // Copy-in parameters keep their static cell for ALL references (§6.1): the entry-time + // copy-in loads the incoming value into that cell, and the body reads and writes it + // there - this also keeps the externally referencing code (defer handler, inline + // assembly) observing the body's current value instead of a stale one. sub.forEachChunk { chunk -> val instrs = chunk.instructions for (i in instrs.indices) { val instr = instrs[i] val mem = instr.memory ?: continue val sym = mem.symbolName ?: continue + if (sym in copyInParams) continue val slot = slotOf(sym, slotBySymbol, paramSlots) ?: continue val newMem = when (mem) { is MemoryReference.Direct -> @@ -672,32 +675,111 @@ class StackFrameLayout(private val program: IRProgram, private val errors: IErro * A framed subroutine cannot branch out to another subroutine: its `link` has already adjusted * the stack pointer, so the other subroutine's `rts` would return into the middle of the local * area. Turn such a tail jump into a call followed by the normal return of this subroutine. + * A conditional branch out (`if cc goto `) becomes an inverted branch over the tail call: + * when the condition holds the call is executed, otherwise the skip label resumes the + * not-taken path with the frame still live. */ private fun rewriteOutgoingJumps(framedSubLabels: Set) { val subLabels = program.allSubs().map { it.label }.toSet() for (sub in program.allSubs()) { if (sub.label !in framedSubLabels) continue + // collect first: rewriting below inserts chunks into the chunk lists of this sub + val rewrites = mutableListOf() sub.forEachChunk { chunk -> - val instrs = chunk.instructions - var i = 0 - while (i < instrs.size) { - val instr = instrs[i] - val target = instr.takeIf { it.opcode == Opcode.JUMP }?.target as? CodeReference.Label - if (target != null && target.name != sub.label && target.name in subLabels) { - instrs[i] = IRInstructions.call(CallSite( - target = CallTarget.Direct(target), - arguments = emptyList() - )) - instrs.add(i + 1, IRInstructions.returnVoid()) - // the RETURN is now the terminator; anything that followed the - // original JUMP in this chunk is unreachable dead code - while (instrs.size > i + 2) - instrs.removeAt(instrs.size - 1) - i++ + chunk.instructions.forEachIndexed { i, instr -> + val target = instr.target as? CodeReference.Label ?: return@forEachIndexed + if (target.name == sub.label || target.name !in subLabels) return@forEachIndexed + when { + instr.opcode == Opcode.JUMP -> rewrites += OutgoingJump(chunk, i, target) + instr.opcode in InvertedBranchOpcodes -> rewrites += OutgoingJump(chunk, i, target) } - i++ } } + if (rewrites.isEmpty()) continue + + val parentLists = mutableListOf>() + fun collectLists(chunks: MutableList) { + parentLists += chunks + chunks.filterIsInstance().forEach { collectLists(it.body) } + } + collectLists(sub.chunks) + + for (rew in rewrites) { + val instrs = rew.chunk.instructions + if (instrs[rew.index].opcode == Opcode.JUMP) { + instrs[rew.index] = IRInstructions.call(CallSite( + target = CallTarget.Direct(rew.target), + arguments = emptyList() + )) + instrs.add(rew.index + 1, IRInstructions.returnVoid()) + // the RETURN is now the terminator; anything that followed the + // original JUMP in this chunk is unreachable dead code + while (instrs.size > rew.index + 2) + instrs.removeAt(instrs.size - 1) + } else { + val skipLabel = nextGeneratedLabel(sub) + // the not-taken path continues in a fresh chunk right after this one + val tail = IRCodeChunk(skipLabel, rew.chunk.next) + tail.instructions.addAll(instrs.subList(rew.index + 1, instrs.size)) + val original = instrs[rew.index] + while (instrs.size > rew.index + 1) + instrs.removeAt(instrs.size - 1) + instrs[rew.index] = invertedBranch(original, skipLabel) + instrs += IRInstructions.call(CallSite( + target = CallTarget.Direct(rew.target), + arguments = emptyList() + )) + instrs += IRInstructions.returnVoid() + rew.chunk.next = null // the chunk now ends with RETURN + val parent = parentLists.first { rew.chunk in it } + parent.add(parent.indexOf(rew.chunk) + 1, tail) + } + } + } + } + + private class OutgoingJump(val chunk: IRCodeChunkBase, val index: Int, val target: CodeReference.Label) + + private var generatedLabelCounter = 0 + + /** unique label scoped under the subroutine, for compiler-generated control flow */ + private fun nextGeneratedLabel(sub: IRSubroutine): String = + "${sub.label}.p8_tailskip${generatedLabelCounter++}" + + private val InvertedStatusBranch = mapOf( + Opcode.BSTCC to Opcode.BSTCS, Opcode.BSTCS to Opcode.BSTCC, + Opcode.BSTEQ to Opcode.BSTNE, Opcode.BSTNE to Opcode.BSTEQ, + Opcode.BSTNEG to Opcode.BSTPOS, Opcode.BSTPOS to Opcode.BSTNEG, + Opcode.BSTVC to Opcode.BSTVS, Opcode.BSTVS to Opcode.BSTVC) + + // immediate-form comparison branches keep their (register, immediate) operands + private val InvertedImmediateBranch = mapOf( + Opcode.BGT to Opcode.BLE, Opcode.BLE to Opcode.BGT, + Opcode.BLT to Opcode.BGE, Opcode.BGE to Opcode.BLT, + Opcode.BGTS to Opcode.BLES, Opcode.BLES to Opcode.BGTS, + Opcode.BLTS to Opcode.BGES, Opcode.BGES to Opcode.BLTS) + + // register-form comparison branches swap both operands: bgtr a,b inverts to bger b,a + // (not(a>b) <=> b>=a); the m68k backend supports all of these opcodes directly + private val InvertedRegisterBranch = mapOf( + Opcode.BGTR to Opcode.BGER, Opcode.BGER to Opcode.BGTR, + Opcode.BGTSR to Opcode.BGESR, Opcode.BGESR to Opcode.BGTSR) + + private val InvertedBranchOpcodes: Set = + InvertedStatusBranch.keys + InvertedImmediateBranch.keys + InvertedRegisterBranch.keys + + /** the same conditional branch with the inverted condition, retargeted to [skipLabel] */ + private fun invertedBranch(instr: IRInstruction, skipLabel: String): IRInstruction { + val skip = codeLabel(skipLabel) + return when (val opcode = instr.opcode) { + in InvertedStatusBranch -> instr.copy(opcode = InvertedStatusBranch.getValue(opcode), target = skip) + in InvertedImmediateBranch -> instr.copy(opcode = InvertedImmediateBranch.getValue(opcode), target = skip) + in InvertedRegisterBranch -> instr.copy( + opcode = InvertedRegisterBranch.getValue(opcode), + srcA = instr.srcB?.copy(role = OperandRole.LEFT), + srcB = instr.srcA?.copy(role = OperandRole.RIGHT), + target = skip) + else -> throw AssemblyError("cannot invert conditional branch ${instr.opcode}") } } diff --git a/compiler/test/TestStackFrameLocals.kt b/compiler/test/TestStackFrameLocals.kt index dc3a9d951..90640e578 100644 --- a/compiler/test/TestStackFrameLocals.kt +++ b/compiler/test/TestStackFrameLocals.kt @@ -335,6 +335,142 @@ main { ir.contains("call p8b_main.p8s_target") shouldBe true } + test("conditional goto to a subroutine out of a framed subroutine becomes an inverted branch over a tail call") { + // the not-taken path must skip the tail call and keep using the frame: the branch + // is inverted over a call + return, and the fall-through code moves into a skip chunk + val src = """ +main { + ubyte @shared result + sub t1() { + result += 10 + } + sub jumper(ubyte n) { + ubyte local + local = n + if local != 0 { + goto t1 + } + result += 1 + } + sub start() { + result = 0 + jumper(1) + jumper(0) + } +} +""" + val (lines, ir) = compile(src) + val jumperAsm = subAssembly(lines, "p8b_main.p8s_jumper") + jumperAsm.any { it.startsWith("link ") } shouldBe true + // no conditional branch directly targeting the other subroutine remains in the frame + // (matching the entry label means the callee's rts would corrupt the machine stack) + jumperAsm.filter { Regex("b(?!sr)\\w+\\s+p8b_main\\.p8s_t1$").containsMatchIn(it) } shouldBe emptyList() + // inverted branch over the tail call: bstne t1 became bsteq , followed by bsr + rts + val tailSkipLabel = jumperAsm.first { Regex("b\\w+\\s+\\S*tailskip").containsMatchIn(it) } + tailSkipLabel.isEmpty() shouldBe false + jumperAsm.any { it == "bsr p8b_main.p8s_t1" } shouldBe true + ir.contains("call p8b_main.p8s_t1") shouldBe true + // the not-taken path resumes after the tail call, still inside the frame + val skipIdx = jumperAsm.indexOfFirst { Regex("\\S*tailskip\\d+:").matches(it) } + (skipIdx > jumperAsm.indexOf("bsr p8b_main.p8s_t1")) shouldBe true + } + + test("conditional goto inside a repeat loop gets the skip chunk inside the loop body") { + val src = """ +main { + ubyte @shared result + sub t1() { + result += 10 + } + sub jumper(ubyte n) { + ubyte local + local = n + repeat 2 { + if local != 0 { + goto t1 + } + } + result += 1 + } + sub start() { + result = 0 + jumper(1) + jumper(0) + } +} +""" + val (lines, ir) = compile(src) + val jumperAsm = subAssembly(lines, "p8b_main.p8s_jumper") + jumperAsm.any { it.startsWith("link ") } shouldBe true + jumperAsm.filter { Regex("b(?!sr)\\w+\\s+p8b_main\\.p8s_t1$").containsMatchIn(it) } shouldBe emptyList() + jumperAsm.any { it.startsWith("dbra d7,") } shouldBe true + jumperAsm.any { it == "bsr p8b_main.p8s_t1" } shouldBe true + ir.contains("call p8b_main.p8s_t1") shouldBe true + } + + test("register-form conditional goto out of a framed subroutine inverts with swapped operands") { + val src = """ +main { + ubyte @shared result + ubyte @shared limit + sub t1() { + result += 10 + } + sub jumper(ubyte n) { + ubyte local + local = n + if local > limit { + goto t1 + } + result += 1 + } + sub start() { + result = 0 + limit = 5 + jumper(9) ; taken: result=10 + jumper(2) ; not taken: result=11 + } +} +""" + val (lines, ir) = compile(src) + val jumperAsm = subAssembly(lines, "p8b_main.p8s_jumper") + jumperAsm.any { it.startsWith("link ") } shouldBe true + // bgtr t1 became bger(swap) -> the inverted condition branches over the tail call + jumperAsm.filter { Regex("b(?!sr)\\w+\\s+p8b_main\\.p8s_t1$").containsMatchIn(it) } shouldBe emptyList() + jumperAsm.count { it.startsWith("bhs") && Regex("\\S*tailskip").containsMatchIn(it) } shouldBe 1 + jumperAsm.any { it == "bsr p8b_main.p8s_t1" } shouldBe true + ir.contains("call p8b_main.p8s_t1") shouldBe true + } + + test("copy-in parameter that the body writes keeps its static cell as the live value") { + // §6.1: the entry-time copy-in turns the static cell into the parameter's live storage; + // the body's writes must land there so external references observe the current value + val src = """ +main { + ubyte @shared out + sub parm(ubyte x) { + x = 99 + defer out = x + } + sub start() { + parm(1) + } +} +""" + val (lines, _) = compile(src) + val parm = subAssembly(lines, "p8b_main.p8s_parm") + parm.any { it == "link a5,#0" } shouldBe true + // the entry-time copy-in is still performed + parm.any { it.startsWith("move.b 11(a5),") } shouldBe true + // the body's write goes to the static cell, not to the incoming frame slot + parm.any { it.contains("move.b") && it.contains("p8b_main.p8s_parm.p8v_x") && it.contains("#99") } shouldBe true + // the defer handler reads the static cell, so it observes the body's written value + val defers = subAssembly(lines, "p8s_prog8_invoke_defers") + defers.any { it.contains("move.b") && it.contains("p8b_main.p8s_parm.p8v_x") } shouldBe true + // the static cell is kept in BSS + lines.any { "p8b_main.p8s_parm.p8v_x:" in it } shouldBe true + } + test("slice 3: recursive subroutine with an address-taken local is rejected") { val src = """ main { @@ -510,9 +646,10 @@ main { rec.any { it == "link a5,#0" } shouldBe true rec.any { it.startsWith("move.b 11(a5),") } shouldBe true rec.any { it.contains("p8b_main.p8s_rec.p8v_x") } shouldBe true - // the static cell is kept for the assembly reference; the body itself uses the frame slot + // the static cell is kept for the assembly reference; the body itself also uses the + // static cell (the entry-time copy-in made it the current value) lines.any { "p8b_main.p8s_rec.p8v_x:" in it } shouldBe true - rec.any { it.startsWith("move.b 11(a5),") && it.endsWith(",p8_regfile+0") } shouldBe true + rec.any { it.startsWith("move.b p8b_main.p8s_rec.p8v_x,") } shouldBe true } test("slice 5: static-initialized local is framed and initialized at entry") {