diff --git a/STATUS.md b/STATUS.md index f310dda..3f19959 100644 --- a/STATUS.md +++ b/STATUS.md @@ -298,7 +298,7 @@ which runs correctly under MAME (apple2gs). insns) disassemble fully coherent. - `runtime/build.sh` builds crt0, libc, soft-float, soft-double, libgcc into linkable objects. -- `scripts/smokeTest.sh` runs 148 end-to-end checks at -O2: +- `scripts/smokeTest.sh` runs ~175 end-to-end checks at -O2: scalar ops, control flow, calling conventions, MAME execution regressions, link816 bss-base safety + weak-symbol resolution + heap_end-vs-heap_start sanity, iigs/toolbox.h compile + link, @@ -455,6 +455,60 @@ Work is now optimization-focused; the toolchain is feature-complete for the common-case C / minimal-C++ workload. Priority is speed (cycle counts), not size. +**Recently landed (2026-07-03):** + +- **StackSlotCleanup flag-wrap correctness fix** — a conditional branch + reads its flags from a real `CMP` (`Defs=[P]`); the pure-logic ops + `AND`/`ORA`/`EOR` set N/Z but are not modeled `Defs=[P]`. When the + scheduler placed one (a loop-invariant `x ^ 0x8000` signed-compare + bias) between a loop-guard `CMP` and its branch, `W65816StackSlotClean` + `up`'s PHP/PLP flag-preservation wrap failed to bracket it — its + corruptor set omitted those opcodes — so the branch saw the `eor`'s Z + instead of the `CMP`'s. A zero-trip loop's guard then *entered* the + loop and spun ~65536 iterations reading out of bounds (a hang / garbage + return). Reported by the external SNES user as a follow-up to the + `caeb78e` SepRepCleanup fix — same *class* (a flag-affecting op the + wrap missed) but a different, sibling pass. Fix: add `AND`/`ORA`/`EOR` + (immediate + absolute forms) to the corruptor set; the `*fi`/`ADC`/ + `SBC` forms do model `Defs=[P]` and are correctly excluded. Scheduling- + independent regression `tests/regress/flagWrapLoopGuard.mir` (a + `-run-pass=w65816-stack-slot-cleanup` unit test) in the smoke suite; + full suite green. A separate `-Oz`-only greedy register-allocator + crash on the same function (1-register `Acc16` recoloring deadlock) is + a known limitation — use `-Os` or `-mllvm -regalloc=basic`; real code + compiles fine at `-Oz`. See `feedback_stackslotcleanup_flagwrap.md`. + +- **`-O0` i8-store-through-pointer wrap made atomic** — an `unsigned + char` value carried across a call and packed via `<< 16` returned + garbage at `-O0` only (same external SNES reporter). Root cause: the + byte store's `SEP #$20 ; sta [dp],y ; REP #$20` wrap was emitted as + three separate pre-RA instructions, so the `-O0` fast register + allocator (which reloads before every use) could insert a 16-bit + reload *inside* the `M=8` window — loading only the low byte of a + 16-bit pointer and corrupting the deref. Fix: emit a single atomic + pseudo (`STB_DPIndLongY` for `[dp],Y`; `STA8fi_indY` for the Layer-2 + `(d,S),Y` form) that stays opaque to register allocation and expands + to real `SEP/STA/REP` MIs post-RA (via `expandPostRAPseudo` / + `eliminateFrameIndex`), mirroring the already-safe frame-index + `STA8fi`. New `-O0` regression `tests/regress/byteStoreO0.c` in the + smoke suite; full suite green. See `feedback_o0_byte_store_wrap.md`. + +**Recently landed (2026-07-02):** + +- **SepRepCleanup PHI-copy-hoist correctness fix** — the hoist's + gap conflict check treated only `LDA_StackRel` as a slot read, so + it could sink a `STA slot` above an `AND/ORA/EOR/ADC/SBC/CMP_ + StackRel` read of that slot. This silently miscompiled a + shared-condition select at every optimized -O level (`best` + clobbered with the select's true value), reported by an external + user building this toolchain to target the **SNES** (the 5A22 is a + WDC 65816). One-site fix in `W65816SepRepCleanup.cpp`; new + regression test `tests/regress/boolSelectHoist.c` in the smoke + suite; full suite green. A separate `-O0`-only miscompile of a + `char` value carried across a call + `<<16` pack was found to be + pre-existing (fixed the next day — see the 2026-07-03 entry above). + See `feedback_boolselect_ssc_miscompile.md`. + **Recently landed (2026-05-25):** - **Layer 1 ptr32 deref-fold (always on)** — Constant offset on a @@ -597,8 +651,20 @@ for the common-case C / minimal-C++ workload. Priority is speed `#LDAs(Block) == #STAs(Block) + #STAs(Trailing)`; an extra LDA is a memory-to-register PHI value live-out at the back-edge (consumed by the loop top's first STA), and hoisting would - clobber A. Saves 2 inst / 8 cyc per occurrence. sumOfSquares - 19096 → 18755 (-1.8%), popcount 3683 → 3478 (-5.6%). + clobber A. (3) **gap conflict check** — walking back to the + insertion point, bail if any in-gap op reads a slot the hoist + writes or writes a slot the hoist reads (using post-unbump + effective offsets). **Correctness fix 2026-07-02:** the "reads a + slot we write" arm originally recognized only `LDA_StackRel` as a + slot read; `AND/ORA/EOR/ADC/SBC/CMP_StackRel` all read op0 as a + slot too, so a `STA slot` could be hoisted above an `AND_StackRel + slot` — silently feeding the AND the just-hoisted value. This + miscompiled a shared-condition select (`best` clobbered with the + select's true value `y`) at all optimized -O levels; reported by + an external user targeting the SNES. Regression test: + `tests/regress/boolSelectHoist.c`. Saves 2 inst / 8 cyc per + occurrence. sumOfSquares 19096 → 18755 (-1.8%), popcount 3683 → + 3478 (-5.6%). - **More peephole / libcall opportunities.** __mulsi3 just gained early-exit when the multiplier shifts to 0; dotProduct dropped diff --git a/scripts/smokeTest.sh b/scripts/smokeTest.sh index b88383c..15ee7ca 100755 --- a/scripts/smokeTest.sh +++ b/scripts/smokeTest.sh @@ -878,6 +878,25 @@ EOF die "i32 shift-by-1 regression" fi + # W65816StackSlotCleanup must bracket a flag-corrupting AND/ORA/EOR that sits + # between a loop-guard CMP and its branch in PHP/PLP, so the branch reads the + # CMP's flags. Its corruptor set (isLdaLike) once omitted the pure-logic + # NZ-setters, so an `eor #0x8000` signed-compare bias hoisted into a zero-trip + # loop's preheader trampled the guard's Z: the guard entered the loop and spun + # ~65536 iterations reading out of bounds (the caeb78e follow-up + # course_floor_piece hang, reported by the external SNES user). The pass + # inserts a PHP only when it wraps -- greping for it distinguishes fixed from + # buggy on a scheduling-independent MIR input. + log "check: W65816StackSlotCleanup wraps a flag-corrupting EOR before a guard branch" + ssMirOut="$(mktemp --suffix=.mir)" + "$LLC" -march=w65816 -run-pass=w65816-stack-slot-cleanup \ + "$PROJECT_ROOT/tests/regress/flagWrapLoopGuard.mir" -o "$ssMirOut" 2>/dev/null + if ! grep -qE '^[[:space:]]*PHP$' "$ssMirOut"; then + cat "$ssMirOut" >&2 + die "StackSlotCleanup did not wrap the flag corruptor (flag-wrap regression)" + fi + rm -f "$ssMirOut" + # Varargs (): LowerFormalArguments creates a fixed FI # for the first vararg slot when IsVarArg; LowerVASTART stores # its address to the va_list pointer. VAARG/VACOPY/VAEND use @@ -1634,7 +1653,27 @@ EOF 0x025000=7fff 0x025002=0003 >/dev/null 2>&1; then die "MAME: boolSelectHoist best != 0x7FFF (SepRepCleanup hoist regression)" fi - rm -f "$oBshFile" "$binBshFile" "$oCrt0File" + rm -f "$oBshFile" "$binBshFile" + + # Regression: -O0 i8-store-through-pointer must keep its SEP/REP wrap + # atomic. Before the fix, the byte store's SEP/STA/REP were three + # separate MIs and the -O0 fast register allocator wedged a 16-bit + # reload into the M=8 window, truncating a live value carried across + # a call. Fix = single STB_DPIndLongY / STA8fi_indY pseudo expanded + # post-RA. MUST compile at -O0 (the bug does not occur at -O1+). + # caller(34) returns 35 + 2 = 37 = 0x0025. + log "check: MAME runs byteStoreO0 regression (-O0 atomic i8-store wrap)" + oBso="$(mktemp --suffix=.o)" + binBso="$(mktemp --suffix=.bin)" + "$CLANG" --target=w65816 -O0 -ffunction-sections -c \ + "$PROJECT_ROOT/tests/regress/byteStoreO0.c" -o "$oBso" + "$PROJECT_ROOT/tools/link816" -o "$binBso" --text-base 0x1000 \ + "$oCrt0File" "$oBso" "$oLibgccFile" 2>/dev/null + if ! bash "$PROJECT_ROOT/scripts/runInMame.sh" "$binBso" --check \ + 0x025000=0025 >/dev/null 2>&1; then + die "MAME: byteStoreO0 != 0x0025 (-O0 byte-store wrap regression)" + fi + rm -f "$oBso" "$binBso" "$oCrt0File" fi # Fuzzer: generate 20 small random C programs and verify all compile. diff --git a/src/llvm/lib/Target/W65816/W65816ISelLowering.cpp b/src/llvm/lib/Target/W65816/W65816ISelLowering.cpp index f9c79a5..4e322cf 100644 --- a/src/llvm/lib/Target/W65816/W65816ISelLowering.cpp +++ b/src/llvm/lib/Target/W65816/W65816ISelLowering.cpp @@ -3115,14 +3115,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::LDY_Imm16)).addImm(0); + // Byte store: one atomic STB_DPIndLongY pseudo (AsmPrinter expands + // to SEP #$20 ; sta [dp],y ; REP #$20). See STB_DPIndLongY in the + // .td: emitting SEP/STA/REP as three MIs let the -O0 fast RA wedge + // a 16-bit reload into the M=8 window, truncating a live value. if (IsByteStore) BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::SEP)).addImm(0x20); - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); - if (IsByteStore) + TII.get(W65816::STB_DPIndLongY)).addImm(0xE0); + else BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::REP)).addImm(0x20); + TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); } MI.eraseFromParent(); return BB; @@ -3385,14 +3387,14 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, Register Val = MI.getOperand(0).getReg(); BuildMI(*BB, MI.getIterator(), DL, TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); - if (IsByteStore) - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::SEP)).addImm(0x20); - BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::STAfi_indY)) + // Byte store: one atomic STA8fi_indY pseudo (eliminateFrameIndex + // adds the SEP/REP wrap post-RA) — see STA8fi_indY in the .td. + // Emitting SEP/STAfi_indY/REP as three MIs let the -O0 fast RA + // wedge a 16-bit reload into the M=8 window. + BuildMI(*BB, MI.getIterator(), DL, + TII.get(IsByteStore ? W65816::STA8fi_indY + : W65816::STAfi_indY)) .addReg(W65816::A).addFrameIndex(FILo).addImm(0); - if (IsByteStore) - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::REP)).addImm(0x20); } MI.eraseFromParent(); if (DeadPtrDef) DeadPtrDef->eraseFromParent(); @@ -3609,14 +3611,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::LDY_Imm16)).addImm(PeelOff); + // Byte store: one atomic STB_DPIndLongY pseudo (AsmPrinter expands + // to SEP #$20 ; sta [dp],y ; REP #$20). See STB_DPIndLongY in the + // .td: emitting SEP/STA/REP as three MIs let the -O0 fast RA wedge + // a 16-bit reload into the M=8 window, truncating a live value. if (IsByteStore) BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::SEP)).addImm(0x20); - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); - if (IsByteStore) + TII.get(W65816::STB_DPIndLongY)).addImm(0xE0); + else BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::REP)).addImm(0x20); + TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); } MI.eraseFromParent(); if (DeadPtrDef) DeadPtrDef->eraseFromParent(); @@ -3699,14 +3703,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::LDY_Imm16)).addImm(Off); + // Byte store: one atomic STB_DPIndLongY pseudo (AsmPrinter expands + // to SEP #$20 ; sta [dp],y ; REP #$20). See STB_DPIndLongY in the + // .td: emitting SEP/STA/REP as three MIs let the -O0 fast RA wedge + // a 16-bit reload into the M=8 window, truncating a live value. if (IsByteStore) BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::SEP)).addImm(0x20); - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); - if (IsByteStore) + TII.get(W65816::STB_DPIndLongY)).addImm(0xE0); + else BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::REP)).addImm(0x20); + TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); } MI.eraseFromParent(); return BB; @@ -3773,14 +3779,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::LDY_Imm16)).addImm(0); + // Byte store: one atomic STB_DPIndLongY pseudo (AsmPrinter expands + // to SEP #$20 ; sta [dp],y ; REP #$20). See STB_DPIndLongY in the + // .td: emitting SEP/STA/REP as three MIs let the -O0 fast RA wedge + // a 16-bit reload into the M=8 window, truncating a live value. if (IsByteStore) BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::SEP)).addImm(0x20); - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); - if (IsByteStore) + TII.get(W65816::STB_DPIndLongY)).addImm(0xE0); + else BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::REP)).addImm(0x20); + TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); } MI.eraseFromParent(); return BB; @@ -3885,14 +3893,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::LDY_Imm16)).addImm(0); + // Byte store: one atomic STB_DPIndLongY pseudo (AsmPrinter expands + // to SEP #$20 ; sta [dp],y ; REP #$20). See STB_DPIndLongY in the + // .td: emitting SEP/STA/REP as three MIs let the -O0 fast RA wedge + // a 16-bit reload into the M=8 window, truncating a live value. if (IsByteStore) BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::SEP)).addImm(0x20); - BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); - if (IsByteStore) + TII.get(W65816::STB_DPIndLongY)).addImm(0xE0); + else BuildMI(*BB, MI.getIterator(), DL, - TII.get(W65816::REP)).addImm(0x20); + TII.get(W65816::STA_DPIndLongY)).addImm(0xE0); } MI.eraseFromParent(); return BB; diff --git a/src/llvm/lib/Target/W65816/W65816InstrInfo.cpp b/src/llvm/lib/Target/W65816/W65816InstrInfo.cpp index 7b6a291..0cf58c9 100644 --- a/src/llvm/lib/Target/W65816/W65816InstrInfo.cpp +++ b/src/llvm/lib/Target/W65816/W65816InstrInfo.cpp @@ -25,6 +25,28 @@ using namespace llvm; void W65816InstrInfo::anchor() {} +// Expand STB_DPIndLongY -> SEP #$20 ; sta [dp],y ; REP #$20. Runs in the +// post-RA pseudo-expansion pass (after the fast/greedy allocator, before +// SepRepCleanup and the AsmPrinter). Keeping the byte store as a single +// pseudo through register allocation is what fixes the -O0 miscompile: the +// fast allocator used to wedge a 16-bit reload between a hand-emitted SEP +// and REP, running it in M=8 and truncating a live value. Expanding here +// (rather than at the AsmPrinter) leaves real SEP/STA/REP MIs so the SEP/ +// REP coalescer and the AsmPrinter's LDAi8imm mode-tracking behave exactly +// as they did for the old three-instruction form. +bool W65816InstrInfo::expandPostRAPseudo(MachineInstr &MI) const { + if (MI.getOpcode() != W65816::STB_DPIndLongY) + return false; + MachineBasicBlock &MBB = *MI.getParent(); + DebugLoc DL = MI.getDebugLoc(); + int64_t Addr = MI.getOperand(0).getImm(); + BuildMI(MBB, MI, DL, get(W65816::SEP)).addImm(0x20); + BuildMI(MBB, MI, DL, get(W65816::STA_DPIndLongY)).addImm(Addr); + BuildMI(MBB, MI, DL, get(W65816::REP)).addImm(0x20); + MI.eraseFromParent(); + return true; +} + W65816InstrInfo::W65816InstrInfo(const W65816Subtarget &STI) : W65816GenInstrInfo(STI, RI, W65816::ADJCALLSTACKDOWN, W65816::ADJCALLSTACKUP), diff --git a/src/llvm/lib/Target/W65816/W65816InstrInfo.h b/src/llvm/lib/Target/W65816/W65816InstrInfo.h index 8744549..ae5a719 100644 --- a/src/llvm/lib/Target/W65816/W65816InstrInfo.h +++ b/src/llvm/lib/Target/W65816/W65816InstrInfo.h @@ -96,6 +96,14 @@ public: // rematerializable — that loads a computed value. bool isReMaterializableImpl(const MachineInstr &MI) const override; + // Post-RA expansion of STB_DPIndLongY (atomic i8-store-through-[dp],Y) + // into SEP #$20 ; sta [dp],y ; REP #$20. Kept as one pseudo through + // register allocation so the -O0 fast allocator cannot insert a 16-bit + // reload inside the M=8 window; expanded here (post-RA, before + // SepRepCleanup and the AsmPrinter) into real MIs so both the SEP/REP + // coalescer and the AsmPrinter's mode-tracking see normal instructions. + bool expandPostRAPseudo(MachineInstr &MI) const override; + // Tell the framework which pseudos are direct stack-slot loads/stores. // MachineCSE, machine-licm, and peephole-opt use these hooks to elide // redundant store/load pairs and to hoist invariants. Without them, diff --git a/src/llvm/lib/Target/W65816/W65816InstrInfo.td b/src/llvm/lib/Target/W65816/W65816InstrInfo.td index 08a252d..ab70174 100644 --- a/src/llvm/lib/Target/W65816/W65816InstrInfo.td +++ b/src/llvm/lib/Target/W65816/W65816InstrInfo.td @@ -1162,6 +1162,17 @@ def STAfi_indY : W65816Pseudo<(outs), (ins Acc16:$src, memfi:$addr), "# STAfi_indY $src, $addr", []>; } +// Atomic i8 (byte) variant of STAfi_indY. Emitted by the STBptr32S +// inserter for Layer-2 (d,S),Y byte stores; eliminateFrameIndex expands +// it with a SEP #$20 / REP #$20 wrap so only one byte is written. Kept a +// single pseudo through register allocation (same reason as STB_DPIndLongY +// / STA8fi): the -O0 fast allocator must not insert a 16-bit reload into +// the M=8 window. Defs P because SEP/REP toggle the M bit. +let mayStore = 1, hasSideEffects = 1, mayLoad = 0, Uses = [Y], Defs = [P] in { +def STA8fi_indY : W65816Pseudo<(outs), (ins Acc16:$src, memfi:$addr), + "# STA8fi_indY $src, $addr", []>; +} + // i8 truncating store via Acc16 pointer. Same shape as STAptr but // custom inserter wraps the actual STA in SEP/REP so the M-bit is 8 // across the store and only one byte is written. Without the wrap the @@ -1175,6 +1186,20 @@ def STBptr : W65816Pseudo<(outs), (ins Acc16:$val, Wide16:$ptr), [(truncstorei8 Acc16:$val, Wide16:$ptr)]>; } +// Atomic i8-store-through-[dp],Y. The STBptr custom inserter emits THIS +// single pseudo (NOT three separate SEP / STA_DPIndLongY / REP MIs) so +// register allocation — especially the -O0 fast allocator, which spills +// everything and reloads before each use — cannot wedge a 16-bit reload +// between the SEP #$20 and REP #$20. Such a reload would run in M=8 and +// load only the low byte of a 16-bit value (it corrupted `px + 1`'s +// pointer reload in the reported -O0 miscompile). The AsmPrinter +// expands this to `SEP #$20 ; sta [dp],y ; REP #$20`. Uses A (value) + +// Y (index 0); Defs P because SEP/REP toggle the M bit. +let mayStore = 1, hasSideEffects = 1, Uses = [A, Y], Defs = [P] in { +def STB_DPIndLongY : W65816Pseudo<(outs), (ins addrDP:$addr), + "# STB_DPIndLongY [${addr}], y", []>; +} + // Pointer access with constant offset. `(load (add ptr, $off))` and // `(store val, (add ptr, $off))` come up for struct field access and // array indexing with small constant offsets. Without these patterns, diff --git a/src/llvm/lib/Target/W65816/W65816RegisterInfo.cpp b/src/llvm/lib/Target/W65816/W65816RegisterInfo.cpp index 60fd02b..64439ad 100644 --- a/src/llvm/lib/Target/W65816/W65816RegisterInfo.cpp +++ b/src/llvm/lib/Target/W65816/W65816RegisterInfo.cpp @@ -428,7 +428,8 @@ bool W65816RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II, case W65816::EORfi: NewOpc = W65816::EOR_StackRel; break; case W65816::CMPfi: NewOpc = W65816::CMP_StackRel; break; case W65816::LDAfi_indY: - case W65816::STAfi_indY: { + case W65816::STAfi_indY: + case W65816::STA8fi_indY: { // (d,S),Y indirect via 8-bit d-byte. If the slot offset fits in // 8 bits, fall through to the normal NewOpc-based lowering below. // Else we need a fallback: long-indirect the slot read via $F6, @@ -436,12 +437,31 @@ bool W65816RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II, // [dp],Y the actual deref. Y is the caller's offset value and // must be preserved across the slot read; stash it at $FA. bool IsLoad = MI.getOpcode() == W65816::LDAfi_indY; + // Byte store (STA8fi_indY): the actual (sr,S),Y / [dp],Y store must + // run in M=8 (one byte). We add the SEP/REP wrap HERE, post-RA, so + // it stays atomic — the -O0 fast allocator can't insert a 16-bit + // reload into the M=8 window (same fix as STB_DPIndLongY / STA8fi). + bool IsByte = MI.getOpcode() == W65816::STA8fi_indY; int FI = MI.getOperand(FIOperandNum).getIndex(); int FrameOffset = MFI.getObjectOffset(FI); int ImmOffset = MI.getOperand(FIOperandNum + 1).getImm(); int Offset = FrameOffset + ImmOffset + (int)MFI.getStackSize() + SPAdj; if (FrameOffset < 0) Offset += 1; if (Offset >= 0 && Offset <= 0xFF && !MFI.hasVarSizedObjects()) { + if (IsByte) { + // SEP #$20 ; sta (Offset,S),Y ; REP #$20 — only one byte written. + MachineBasicBlock &MBB = *MI.getParent(); + DebugLoc DL = MI.getDebugLoc(); + BuildMI(MBB, II, DL, TII.get(W65816::SEP)).addImm(0x20) + .addReg(W65816::P, RegState::ImplicitDefine); + BuildMI(MBB, II, DL, TII.get(W65816::STA_StackRelIndY)).addImm(Offset) + .addReg(W65816::A, RegState::Implicit) + .addReg(W65816::Y, RegState::Implicit); + BuildMI(MBB, II, DL, TII.get(W65816::REP)).addImm(0x20) + .addReg(W65816::P, RegState::ImplicitDefine); + MI.eraseFromParent(); + return true; + } NewOpc = IsLoad ? W65816::LDA_StackRelIndY : W65816::STA_StackRelIndY; break; @@ -494,9 +514,16 @@ bool W65816RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II, // Reload A. BuildMI(MBB, II, DL, TII.get(W65816::LDA_DP)).addImm(0xFC) .addReg(W65816::A, RegState::ImplicitDefine); + // Byte store: wrap the [dp],Y store in SEP/REP (one byte). + if (IsByte) + BuildMI(MBB, II, DL, TII.get(W65816::SEP)).addImm(0x20) + .addReg(W65816::P, RegState::ImplicitDefine); BuildMI(MBB, II, DL, TII.get(W65816::STA_DPIndLongY)).addImm(0xE0) .addReg(W65816::A, RegState::Implicit) .addReg(W65816::Y, RegState::Implicit); + if (IsByte) + BuildMI(MBB, II, DL, TII.get(W65816::REP)).addImm(0x20) + .addReg(W65816::P, RegState::ImplicitDefine); } MI.eraseFromParent(); return true; diff --git a/src/llvm/lib/Target/W65816/W65816StackSlotCleanup.cpp b/src/llvm/lib/Target/W65816/W65816StackSlotCleanup.cpp index 51accc8..159007e 100644 --- a/src/llvm/lib/Target/W65816/W65816StackSlotCleanup.cpp +++ b/src/llvm/lib/Target/W65816/W65816StackSlotCleanup.cpp @@ -906,6 +906,29 @@ bool W65816StackSlotCleanup::runOnMachineFunction(MachineFunction &MF) { // Pseudo wrappers that lower to LDA #imm. Opc == W65816::LDAfi || Opc == W65816::LDAfi_indY || + // Pure-logic NZ-setters: AND / ORA / EOR against an immediate + // or absolute operand. These physically set N/Z but + // deliberately do NOT model Defs=[P] (see W65816InstrInfo.td: + // the bitwise ops "don't read/write the carry flag"), so a + // conditional branch can NEVER take its modeled $p from one of + // them — at this pass's point every branch's flags come from a + // real CMP (Defs=[P]); the CMP-vs-AND/ORA fold that drops the + // `cmp #0` runs later. So when one of these is scheduled + // BETWEEN a loop-guard CMP and its branch (e.g. a loop- + // invariant signed-compare bias `contactY ^ 0x8000` hoisted + // into a zero-trip loop's preheader), it silently tramples the + // CMP's Z: the guard then branches on the wrong value, enters a + // should-be-skipped loop, and spins ~65536 iterations reading + // out-of-bounds data (the reported course_support_piece hang). + // Treating them as corruption walks the wrap past them to the + // real CMP. NOTE: the *fi variants (ANDfi/ORAfi/EORfi) and + // ADC/SBC DO model Defs=[P] and can legitimately be the branch's + // flag source, so they are handled by $p liveness and are NOT + // listed here; same for INA/DEA and the shifts, which the DEC/ + // INC and shift-carry idioms may read directly. + Opc == W65816::EORi16imm || Opc == W65816::EORabs || + Opc == W65816::ANDi16imm || Opc == W65816::ANDabs || + Opc == W65816::ORAi16imm || Opc == W65816::ORAabs || // Register transfers — TAX/TXA/TAY/TYA/TXY/TYX update N/Z // based on the transferred value. They're "data movement" // not "comparison"; treat as corruption so the wrap pass diff --git a/tests/regress/byteStoreO0.c b/tests/regress/byteStoreO0.c new file mode 100644 index 0000000..4ce1b6c --- /dev/null +++ b/tests/regress/byteStoreO0.c @@ -0,0 +1,37 @@ +// Regression test for the -O0 byte-store-through-pointer miscompile +// (W65816 STB_DPIndLongY atomic pseudo). +// +// The i8 store `*pcout = ...` through a ptr32 lowers to a [dp],Y store +// wrapped in SEP #$20 / STA / REP #$20 (M=8 so only one byte is written). +// The custom inserter used to emit those as THREE separate instructions, +// so the -O0 fast register allocator (which spills everything and reloads +// before each use) could wedge a 16-bit reload BETWEEN the SEP and REP — +// running it in M=8, it loaded only the low byte of a 16-bit value. That +// truncated `probe`'s `px + 1` pointer reload, so a value carried across +// the call came back wrong (reported by an external SNES user: an +// `unsigned char pid` packed via `<< 16` returned 0). +// +// Fix: emit ONE atomic STB_DPIndLongY pseudo, expanded to SEP/STA/REP at +// the AsmPrinter, so nothing can be inserted inside the M=8 window. +// +// caller(34): probe writes pc = 34 & 7 = 2 through the pointer and returns +// 34 + 1 = 35; caller returns 35 + 2 = 37 = 0x25. The buggy -O0 build +// returned 3 (probe's return truncated to 1, + pc 2). MUST be compiled +// at -O0 (the bug does not occur at -O1+). + +__attribute__((noinline)) static short probe(short px, unsigned char *pcout) { + *pcout = (unsigned char)(px & 7); + return (short)(px + 1); +} + +__attribute__((noinline)) static short caller(short px) { + unsigned char pc; + short y = probe(px, &pc); // y (probe's return) is live across the call + return (short)(y + pc); // + pc, the byte written through the pointer +} + +int main(void) { + *(volatile unsigned short *)0x025000 = (unsigned short)caller(34); // 0x0025 + while (1) {} + return 0; +} diff --git a/tests/regress/flagWrapLoopGuard.mir b/tests/regress/flagWrapLoopGuard.mir new file mode 100644 index 0000000..e01457e --- /dev/null +++ b/tests/regress/flagWrapLoopGuard.mir @@ -0,0 +1,57 @@ +# Regression test for the W65816StackSlotCleanup flag-preservation wrap +# (caeb78e follow-up: course_floor_piece hang / "reads uninitialized memory"). +# +# RUN (see scripts/smokeTest.sh): +# llc -march=w65816 -run-pass=w65816-stack-slot-cleanup flagWrapLoopGuard.mir +# +# This is the exact miscompiling shape: a loop-guard test `CMPi16imm $a, 0' +# (e.g. `mi < c->nmovers` with nmovers==0) whose branch reads $p, with a +# loop-invariant signed-compare bias `LDAfi contactY; EORi16imm #0x8000' +# scheduled BETWEEN the CMP and the branch. LDA/EOR set N/Z on hardware but +# are NOT modeled with Defs=[P], so they physically trample the CMP's Z. The +# StackSlotCleanup pass must bracket both in PHP/PLP so BEQ still sees the CMP's +# flags. Before the fix its corruptor set (isLdaLike) omitted EORi16imm, so it +# stopped the backward walk AT the EOR (misclassifying it as the "test"), found +# no corruptor between it and BEQ, and inserted NO wrap -- the guard branched on +# (contactY^0x8000)==0 instead of nmovers==0, entered the zero-trip loop, and +# spun ~65536 iterations reading out of bounds. +# +# EXPECTED after the fix: a PHP is inserted before the LDAfi and a PLP after the +# EORi16imm (the smoke check greps for `PHP` between the CMP and the BEQ). +# Without the fix there is no PHP/PLP at all. +--- | + target datalayout = "e-m:e-p:32:16-i16:16-i32:16-i64:16-f32:16-f64:16-a:8-n8:16-S8" + target triple = "w65816" + define void @flagWrapLoopGuard() { ret void } +... +--- +name: flagWrapLoopGuard +alignment: 1 +tracksRegLiveness: true +frameInfo: + maxAlignment: 2 + adjustsStack: true +fixedStack: + - { id: 0, type: default, offset: 4, size: 2, alignment: 1, isImmutable: true } +stack: + - { id: 0, name: '', type: spill-slot, offset: 0, size: 2, alignment: 2 } +body: | + bb.0: + successors: %bb.1(0x40000000), %bb.2(0x40000000) + liveins: $a + ; loop-guard test: nmovers == 0 (the value the BEQ must actually branch on) + CMPi16imm $a, 0, implicit-def $p + ; loop-invariant signed-compare bias scheduled into the guard gap; these + ; physically set N/Z but are not modeled Defs=[P] -> must be PHP/PLP-wrapped + $a = LDAfi %fixed-stack.0, 0 + $a = EORi16imm killed $a, -32768 + STAfi killed $a, %stack.0, 0, implicit-def $a + BEQ %bb.2, implicit $p + BRA %bb.1 + + bb.1: + RTS + + bb.2: + RTS +...