Another miscompile fixed.

This commit is contained in:
Scott Duensing 2026-07-03 17:57:10 -05:00
parent caeb78e0d9
commit fb2f032923
10 changed files with 351 additions and 37 deletions

View file

@ -298,7 +298,7 @@ which runs correctly under MAME (apple2gs).
insns) disassemble fully coherent. insns) disassemble fully coherent.
- `runtime/build.sh` builds crt0, libc, soft-float, soft-double, - `runtime/build.sh` builds crt0, libc, soft-float, soft-double,
libgcc into linkable objects. 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 scalar ops, control flow, calling conventions, MAME execution
regressions, link816 bss-base safety + weak-symbol resolution + regressions, link816 bss-base safety + weak-symbol resolution +
heap_end-vs-heap_start sanity, iigs/toolbox.h compile + link, 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 for the common-case C / minimal-C++ workload. Priority is speed
(cycle counts), not size. (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):** **Recently landed (2026-05-25):**
- **Layer 1 ptr32 deref-fold (always on)** — Constant offset on a - **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 `#LDAs(Block) == #STAs(Block) + #STAs(Trailing)`; an extra LDA
is a memory-to-register PHI value live-out at the back-edge is a memory-to-register PHI value live-out at the back-edge
(consumed by the loop top's first STA), and hoisting would (consumed by the loop top's first STA), and hoisting would
clobber A. Saves 2 inst / 8 cyc per occurrence. sumOfSquares clobber A. (3) **gap conflict check** — walking back to the
19096 → 18755 (-1.8%), popcount 3683 → 3478 (-5.6%). 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 - **More peephole / libcall opportunities.** __mulsi3 just gained
early-exit when the multiplier shifts to 0; dotProduct dropped early-exit when the multiplier shifts to 0; dotProduct dropped

View file

@ -878,6 +878,25 @@ EOF
die "i32 shift-by-1 regression" die "i32 shift-by-1 regression"
fi 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 (<stdarg.h>): LowerFormalArguments creates a fixed FI # Varargs (<stdarg.h>): LowerFormalArguments creates a fixed FI
# for the first vararg slot when IsVarArg; LowerVASTART stores # for the first vararg slot when IsVarArg; LowerVASTART stores
# its address to the va_list pointer. VAARG/VACOPY/VAEND use # 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 0x025000=7fff 0x025002=0003 >/dev/null 2>&1; then
die "MAME: boolSelectHoist best != 0x7FFF (SepRepCleanup hoist regression)" die "MAME: boolSelectHoist best != 0x7FFF (SepRepCleanup hoist regression)"
fi 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 fi
# Fuzzer: generate 20 small random C programs and verify all compile. # Fuzzer: generate 20 small random C programs and verify all compile.

View file

@ -3115,14 +3115,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); TII.get(TargetOpcode::COPY), W65816::A).addReg(Val);
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::LDY_Imm16)).addImm(0); 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) if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::SEP)).addImm(0x20); TII.get(W65816::STB_DPIndLongY)).addImm(0xE0);
BuildMI(*BB, MI.getIterator(), DL, else
TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::REP)).addImm(0x20); TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
} }
MI.eraseFromParent(); MI.eraseFromParent();
return BB; return BB;
@ -3385,14 +3387,14 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
Register Val = MI.getOperand(0).getReg(); Register Val = MI.getOperand(0).getReg();
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); TII.get(TargetOpcode::COPY), W65816::A).addReg(Val);
if (IsByteStore) // Byte store: one atomic STA8fi_indY pseudo (eliminateFrameIndex
BuildMI(*BB, MI.getIterator(), DL, // adds the SEP/REP wrap post-RA) — see STA8fi_indY in the .td.
TII.get(W65816::SEP)).addImm(0x20); // Emitting SEP/STAfi_indY/REP as three MIs let the -O0 fast RA
BuildMI(*BB, MI.getIterator(), DL, TII.get(W65816::STAfi_indY)) // 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); .addReg(W65816::A).addFrameIndex(FILo).addImm(0);
if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::REP)).addImm(0x20);
} }
MI.eraseFromParent(); MI.eraseFromParent();
if (DeadPtrDef) DeadPtrDef->eraseFromParent(); if (DeadPtrDef) DeadPtrDef->eraseFromParent();
@ -3609,14 +3611,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); TII.get(TargetOpcode::COPY), W65816::A).addReg(Val);
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::LDY_Imm16)).addImm(PeelOff); 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) if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::SEP)).addImm(0x20); TII.get(W65816::STB_DPIndLongY)).addImm(0xE0);
BuildMI(*BB, MI.getIterator(), DL, else
TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::REP)).addImm(0x20); TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
} }
MI.eraseFromParent(); MI.eraseFromParent();
if (DeadPtrDef) DeadPtrDef->eraseFromParent(); if (DeadPtrDef) DeadPtrDef->eraseFromParent();
@ -3699,14 +3703,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); TII.get(TargetOpcode::COPY), W65816::A).addReg(Val);
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::LDY_Imm16)).addImm(Off); 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) if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::SEP)).addImm(0x20); TII.get(W65816::STB_DPIndLongY)).addImm(0xE0);
BuildMI(*BB, MI.getIterator(), DL, else
TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::REP)).addImm(0x20); TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
} }
MI.eraseFromParent(); MI.eraseFromParent();
return BB; return BB;
@ -3773,14 +3779,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); TII.get(TargetOpcode::COPY), W65816::A).addReg(Val);
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::LDY_Imm16)).addImm(0); 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) if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::SEP)).addImm(0x20); TII.get(W65816::STB_DPIndLongY)).addImm(0xE0);
BuildMI(*BB, MI.getIterator(), DL, else
TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::REP)).addImm(0x20); TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
} }
MI.eraseFromParent(); MI.eraseFromParent();
return BB; return BB;
@ -3885,14 +3893,16 @@ W65816TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
TII.get(TargetOpcode::COPY), W65816::A).addReg(Val); TII.get(TargetOpcode::COPY), W65816::A).addReg(Val);
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::LDY_Imm16)).addImm(0); 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) if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::SEP)).addImm(0x20); TII.get(W65816::STB_DPIndLongY)).addImm(0xE0);
BuildMI(*BB, MI.getIterator(), DL, else
TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
if (IsByteStore)
BuildMI(*BB, MI.getIterator(), DL, BuildMI(*BB, MI.getIterator(), DL,
TII.get(W65816::REP)).addImm(0x20); TII.get(W65816::STA_DPIndLongY)).addImm(0xE0);
} }
MI.eraseFromParent(); MI.eraseFromParent();
return BB; return BB;

View file

@ -25,6 +25,28 @@ using namespace llvm;
void W65816InstrInfo::anchor() {} 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) W65816InstrInfo::W65816InstrInfo(const W65816Subtarget &STI)
: W65816GenInstrInfo(STI, RI, W65816::ADJCALLSTACKDOWN, : W65816GenInstrInfo(STI, RI, W65816::ADJCALLSTACKDOWN,
W65816::ADJCALLSTACKUP), W65816::ADJCALLSTACKUP),

View file

@ -96,6 +96,14 @@ public:
// rematerializable — that loads a computed value. // rematerializable — that loads a computed value.
bool isReMaterializableImpl(const MachineInstr &MI) const override; 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. // Tell the framework which pseudos are direct stack-slot loads/stores.
// MachineCSE, machine-licm, and peephole-opt use these hooks to elide // MachineCSE, machine-licm, and peephole-opt use these hooks to elide
// redundant store/load pairs and to hoist invariants. Without them, // redundant store/load pairs and to hoist invariants. Without them,

View file

@ -1162,6 +1162,17 @@ def STAfi_indY : W65816Pseudo<(outs), (ins Acc16:$src, memfi:$addr),
"# STAfi_indY $src, $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 // 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 // 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 // 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)]>; [(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 // Pointer access with constant offset. `(load (add ptr, $off))` and
// `(store val, (add ptr, $off))` come up for struct field access and // `(store val, (add ptr, $off))` come up for struct field access and
// array indexing with small constant offsets. Without these patterns, // array indexing with small constant offsets. Without these patterns,

View file

@ -428,7 +428,8 @@ bool W65816RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
case W65816::EORfi: NewOpc = W65816::EOR_StackRel; break; case W65816::EORfi: NewOpc = W65816::EOR_StackRel; break;
case W65816::CMPfi: NewOpc = W65816::CMP_StackRel; break; case W65816::CMPfi: NewOpc = W65816::CMP_StackRel; break;
case W65816::LDAfi_indY: 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 // (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. // 8 bits, fall through to the normal NewOpc-based lowering below.
// Else we need a fallback: long-indirect the slot read via $F6, // 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 // [dp],Y the actual deref. Y is the caller's offset value and
// must be preserved across the slot read; stash it at $FA. // must be preserved across the slot read; stash it at $FA.
bool IsLoad = MI.getOpcode() == W65816::LDAfi_indY; 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 FI = MI.getOperand(FIOperandNum).getIndex();
int FrameOffset = MFI.getObjectOffset(FI); int FrameOffset = MFI.getObjectOffset(FI);
int ImmOffset = MI.getOperand(FIOperandNum + 1).getImm(); int ImmOffset = MI.getOperand(FIOperandNum + 1).getImm();
int Offset = FrameOffset + ImmOffset + (int)MFI.getStackSize() + SPAdj; int Offset = FrameOffset + ImmOffset + (int)MFI.getStackSize() + SPAdj;
if (FrameOffset < 0) Offset += 1; if (FrameOffset < 0) Offset += 1;
if (Offset >= 0 && Offset <= 0xFF && !MFI.hasVarSizedObjects()) { 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 NewOpc = IsLoad ? W65816::LDA_StackRelIndY
: W65816::STA_StackRelIndY; : W65816::STA_StackRelIndY;
break; break;
@ -494,9 +514,16 @@ bool W65816RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
// Reload A. // Reload A.
BuildMI(MBB, II, DL, TII.get(W65816::LDA_DP)).addImm(0xFC) BuildMI(MBB, II, DL, TII.get(W65816::LDA_DP)).addImm(0xFC)
.addReg(W65816::A, RegState::ImplicitDefine); .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) BuildMI(MBB, II, DL, TII.get(W65816::STA_DPIndLongY)).addImm(0xE0)
.addReg(W65816::A, RegState::Implicit) .addReg(W65816::A, RegState::Implicit)
.addReg(W65816::Y, 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(); MI.eraseFromParent();
return true; return true;

View file

@ -906,6 +906,29 @@ bool W65816StackSlotCleanup::runOnMachineFunction(MachineFunction &MF) {
// Pseudo wrappers that lower to LDA #imm. // Pseudo wrappers that lower to LDA #imm.
Opc == W65816::LDAfi || Opc == W65816::LDAfi ||
Opc == W65816::LDAfi_indY || 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 // Register transfers — TAX/TXA/TAY/TYA/TXY/TYX update N/Z
// based on the transferred value. They're "data movement" // based on the transferred value. They're "data movement"
// not "comparison"; treat as corruption so the wrap pass // not "comparison"; treat as corruption so the wrap pass

View file

@ -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;
}

View file

@ -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
...