diff --git a/scripts/smokeTest.sh b/scripts/smokeTest.sh index 5080d18..b88383c 100755 --- a/scripts/smokeTest.sh +++ b/scripts/smokeTest.sh @@ -1612,7 +1612,29 @@ EOF >/dev/null 2>&1; then die "soft-double MAME: 1.5+2.5 != 4.0 (bit pattern wrong)" fi - rm -f "$cDblMame" "$oDblMame" "$oSdMame" "$binDblMame" "$oCrt0File" + rm -f "$cDblMame" "$oDblMame" "$oSdMame" "$binDblMame" + + # Regression: W65816SepRepCleanup's PHI-copy hoist must not move a + # `STA slot` above an `AND/ORA/EOR/ADC/SBC/CMP_StackRel slot` read in + # the gap. Before the fix (gap check only guarded LDA_StackRel), + # course_support_piece miscompiled `best` to `y` at every optimized + # -O level: the hoisted STA staged a select's TRUE value into the + # slot that a predicate AND then read. Reported by an external SNES + # user (gist gingerbeardman/74e25c6a). course_support_piece is + # external + called twice so it stays out-of-line (a single inlined + # call site hides the bug). Must return 0x00037FFF. + log "check: MAME runs boolSelectHoist regression (SepRepCleanup gap check)" + oBshFile="$(mktemp --suffix=.o)" + binBshFile="$(mktemp --suffix=.bin)" + "$CLANG" --target=w65816 -O2 -ffunction-sections -c \ + "$PROJECT_ROOT/tests/regress/boolSelectHoist.c" -o "$oBshFile" + "$PROJECT_ROOT/tools/link816" -o "$binBshFile" --text-base 0x1000 \ + "$oCrt0File" "$oBshFile" "$oLibgccFile" 2>/dev/null + if ! bash "$PROJECT_ROOT/scripts/runInMame.sh" "$binBshFile" --check \ + 0x025000=7fff 0x025002=0003 >/dev/null 2>&1; then + die "MAME: boolSelectHoist best != 0x7FFF (SepRepCleanup hoist regression)" + fi + rm -f "$oBshFile" "$binBshFile" "$oCrt0File" fi # Fuzzer: generate 20 small random C programs and verify all compile. diff --git a/src/llvm/lib/Target/W65816/W65816SepRepCleanup.cpp b/src/llvm/lib/Target/W65816/W65816SepRepCleanup.cpp index bb3e61b..c1a48fa 100644 --- a/src/llvm/lib/Target/W65816/W65816SepRepCleanup.cpp +++ b/src/llvm/lib/Target/W65816/W65816SepRepCleanup.cpp @@ -605,7 +605,22 @@ bool W65816SepRepCleanup::runOnMachineFunction(MachineFunction &MF) { int64_t off = Back->getOperand(0).getImm(); if (ReadSlots.count(off)) { gapOK = false; break; } } - if (BO == W65816::LDA_StackRel && + // Any *_StackRel that READS its slot operand — not just LDA. + // AND/ORA/EOR/ADC/SBC/CMP_StackRel all take op0 as a stack + // slot and read it. Hoisting a Block/Trailing STA above such + // a read of a WriteSlot makes the read observe the hoisted NEW + // value instead of the value live at its original position. + // This was the course_support_piece miscompile: an + // `AND_StackRel 47` (the boolean operand of a select predicate) + // sat in the gap while the wrap's trailing `STA_StackRel 47` + // staged the best-select TRUE value; hoisting that STA above + // the AND fed the AND the true value (y) instead of the + // boolean, so `best` wrongly became y at all optimized -O + // levels. (Previously only LDA_StackRel was checked here.) + if ((BO == W65816::LDA_StackRel || BO == W65816::AND_StackRel || + BO == W65816::ORA_StackRel || BO == W65816::EOR_StackRel || + BO == W65816::ADC_StackRel || BO == W65816::SBC_StackRel || + BO == W65816::CMP_StackRel) && Back->getNumOperands() >= 1 && Back->getOperand(0).isImm()) { int64_t off = Back->getOperand(0).getImm(); if (WriteSlots.count(off)) { gapOK = false; break; } diff --git a/tests/regress/boolSelectHoist.c b/tests/regress/boolSelectHoist.c new file mode 100644 index 0000000..4b467ba --- /dev/null +++ b/tests/regress/boolSelectHoist.c @@ -0,0 +1,156 @@ +// Regression test for the W65816 SepRepCleanup PHI-copy-hoist fix +// (W65816SepRepCleanup.cpp gap conflict-check). +// +// Reported by an external user targeting the SNES (gist gingerbeardman/ +// 74e25c6a). SepRepCleanup's "sink LDA;STA out of a PHP/PLP flag-wrap" +// peephole hoisted a wrap's trailing `STA_StackRel N` (staging a select's +// TRUE value `y`) ABOVE an `AND_StackRel N` in the gap (the select +// predicate's boolean operand) -- its conflict check only treated +// LDA_StackRel as a slot read, missing AND/ORA/EOR/ADC/SBC/CMP_StackRel. +// The AND then read `y` instead of the boolean, so `best` wrongly became +// `y` at every optimized -O level. +// +// course_support_piece(&gCourse, 32, 250, 8, 0, 0) must return 0x00037FFF +// (best stays 0x7FFF, pid = 3). The buggy backend returned 0x00030082 +// (best clobbered with y = 130). course_support_piece MUST stay +// out-of-line (external + 2 call sites) -- a single inlined call site +// changes register allocation and hides the bug. Verified in MAME. +typedef short fx_i16; +typedef unsigned short fx_u16; +typedef unsigned char fx_u8; +typedef signed char fx_i8; +typedef unsigned long fx_u32; // 32-bit on w65816 + +#define NO_GROUND 0x7FFF +#define MASK_NONE 0x7FFF +#define FLAT_SLOPED 0x7FFE +#define CTILE 16 +#define CTSHIFT 4 +#define CTMASK 15 +#define PC_GAP 0 +#define PC_CRUMBLE 2 +#define PIECE_SHIFT 4 + +#define fx_shr16(n, k) ((fx_i16)((fx_i16)(n) >> (k))) +#define fx_abs16(n) ((fx_i16)((n) < 0 ? -(n) : (n))) +#define rd_i16(p, i) (((const volatile fx_i16 *)(p))[(i)]) +#define rd_u16(p, i) (((const volatile fx_u16 *)(p))[(i)]) +#define rd_u8(p, i) (((const volatile fx_u8 *)(p))[(i)]) + +typedef struct { + fx_i16 start_tcol, end_tcol; + fx_i16 start_col, end_col; + const fx_u16 *tmask; + const fx_i16 *tbase; + const fx_u8 *ground_piece; + const fx_i16 *col_surf_flat; +} course_layer_t; + +typedef struct { + int nlayers; + const course_layer_t *layers; + const fx_i16 *masks; + const fx_u8 *collapsed; + fx_i16 max_px; +} course_t; + +static __attribute__((noinline)) fx_i16 wall_clamp_px(const course_t *c, fx_i16 px) { + if (px < 0) { return 0; } + if (px > c->max_px) { return c->max_px; } + return px; +} + +static fx_i16 layer_probe(const course_t *c, const course_layer_t *L, + fx_i16 px, fx_i16 col, int li, fx_u8 *piece_out) { + (void)li; + fx_i16 ret; fx_u8 outpiece; + fx_i16 tcol = fx_shr16(px, CTSHIFT); + if (tcol < L->start_tcol || tcol >= L->end_tcol) { ret = NO_GROUND; outpiece = PC_GAP; } + else { + int ti = tcol - L->start_tcol; + int in_col = (col >= L->start_col && col < L->end_col); + int gi = col - L->start_col; + const fx_u16 *tmask = L->tmask; const fx_i16 *tbase = L->tbase; + const fx_u8 *gp = L->ground_piece; const fx_i16 *masks = c->masks; + const fx_u8 *collapsed = c->collapsed; const fx_i16 *csf = L->col_surf_flat; + fx_u8 mi = (fx_u8)(px & CTMASK); fx_u8 gpv = PC_GAP; + { fx_i16 fy = rd_i16(csf, ti); + if (fy != FLAT_SLOPED) { + if (in_col) gpv = rd_u8(gp, gi); + if (piece_out) *piece_out = in_col ? gpv : PC_GAP; + return fy; + } } + if (in_col) gpv = rd_u8(gp, gi); + fx_u16 id = rd_u16(tmask, ti); + if (id == 0) ret = NO_GROUND; + else if (collapsed && in_col && collapsed[col] && gpv == PC_CRUMBLE) ret = NO_GROUND; + else { + fx_i16 off = rd_i16(masks, (int)id * CTILE + mi); + ret = (off == MASK_NONE) ? NO_GROUND : (fx_i16)(rd_i16(tbase, ti) + off); + } + outpiece = in_col ? gpv : PC_GAP; + } + if (piece_out) *piece_out = outpiece; + return ret; +} + +fx_u32 course_support_piece(const course_t *c, fx_i16 px, fx_i16 contactY, fx_i8 grab, + fx_i16 belowFrom, fx_i16 *below_out) { + fx_i16 best = NO_GROUND, bestd = (fx_i16)(grab + 1), piece_bestd = 0x7FFF; + fx_u8 pid = PC_GAP; fx_i16 below = NO_GROUND; + fx_i16 qpx = wall_clamp_px(c, px); + fx_i16 col = fx_shr16(qpx, PIECE_SHIFT); + for (int li = 0; li < c->nlayers; li++) { + const course_layer_t *L = &c->layers[li]; + fx_u8 pc; fx_i16 y = layer_probe(c, L, qpx, col, li, &pc); + if (y != NO_GROUND) { + fx_i16 d = fx_abs16((fx_i16)(y - contactY)); + if (d <= grab && d < bestd) { best = y; bestd = d; } + if (d < piece_bestd) { piece_bestd = d; pid = pc; } + if (below_out) { if (y >= belowFrom && y < below) below = y; } + else if (d == 0) return (fx_u32)(fx_u16)y | ((fx_u32)pc << 16); + } + } + if (below_out) *below_out = below; + return (fx_u32)(fx_u16)best | ((fx_u32)pid << 16); +} + +#define FS FLAT_SLOPED +static const fx_u16 tmask0[16] = {0,1,2,3, 1,2,3,0, 1,2,3,1, 2,3,0,1}; +static const fx_i16 tbase0[16] = {100,110,120,130, 140,150,160,170, 180,190,200,210, 220,230,240,250}; +static const fx_u8 gp0[16] = {1,2,3,4, 5,6,7,8, 1,2,3,4, 5,6,7,8}; +static const fx_i16 csf0[16] = {FS,250,FS,260, FS,FS,250,FS, 300,FS,FS,250, FS,FS,FS,FS}; +static const fx_u16 tmask1[16] = {1,0,2,0, 3,1,0,2, 0,3,1,0, 2,0,3,1}; +static const fx_i16 tbase1[16] = {200,205,210,215, 220,225,230,235, 240,245,250,255, 260,265,270,275}; +static const fx_u8 gp1[16] = {2,2,2,2, 2,2,2,2, 3,3,3,3, 1,1,1,1}; +static const fx_i16 csf1[16] = {FS,FS,FS,FS, 250,FS,FS,FS, FS,240,FS,FS, FS,FS,250,FS}; +static const fx_u16 tmask2[16] = {3,3,0,1, 2,3,0,1, 2,3,0,1, 2,3,0,0}; +static const fx_i16 tbase2[16] = {150,150,150,150, 160,160,160,160, 170,170,170,170, 180,180,180,180}; +static const fx_u8 gp2[16] = {4,4,4,4, 5,5,5,5, 6,6,6,6, 7,7,7,7}; +static const fx_i16 csf2[16] = {FS,FS,FS,FS, FS,FS,FS,FS, FS,FS,FS,FS, FS,FS,FS,FS}; +#undef FS +static const fx_i16 masks[64] = { + 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, + 0,1,2,3, 4,5,6,7, MASK_NONE,0,1,2, 3,4,5,6, + 10,11,MASK_NONE,13, 14,15,16,17, 18,MASK_NONE,20,21, 22,23,24,25, + MASK_NONE,1,2,MASK_NONE, 4,5,6,7, 8,9,10,11, 12,13,14,15 +}; +static const fx_u8 collapsed[16] = {0,0,1,0, 0,1,0,0, 1,0,0,1, 0,0,1,0}; +static const course_layer_t layers[3] = { + { 0,16, 0,16, tmask0, tbase0, gp0, csf0 }, + { 0,16, 0,16, tmask1, tbase1, gp1, csf1 }, + { 0,16, 0,16, tmask2, tbase2, gp2, csf2 }, +}; +static const course_t gCourse = { 3, layers, masks, collapsed, 255 }; + +// Two call sites keep course_support_piece out-of-line (it is far over the +// inline threshold), reproducing the real function's register pressure. +int main(void) { + fx_u32 r = course_support_piece(&gCourse, 32, 250, 8, 0, 0); + fx_u32 r2 = course_support_piece(&gCourse, 48, 260, 8, 0, 0); + *(volatile unsigned short *)0x025000 = (unsigned short)(r & 0xFFFF); + *(volatile unsigned short *)0x025002 = (unsigned short)((r >> 16) & 0xFFFF); + *(volatile unsigned short *)0x025004 = (unsigned short)(r2 & 0xFFFF); + while (1) {} + return 0; +}