christopher/Syphon_Filter_3
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

phase11: CORRECTION — the Phase 10 #nop-honouring fix was TOO BROAD; now opt-in (maspsx=nopmarker)

Browse Source 
Worker C found the defect while characterising an above-ceiling row: the Phase 10 fix
that made maspsx honour cc1's explicit `#nop` marker unconditionally is wrong for a
bare-symbol store consumer, because the store's own `lui $at` expansion fills the
delay slot and the marker is SPURIOUS. On 0x800AFDBC the original is
`lhu` / `lui at` / `sh` with NO nop, and honouring the marker costs 2 instructions --
turning a row that was otherwise byte-identical into a LENGTH-MISMATCH.

But the fix is genuinely needed for 0x80107C5C (112 vs 108). So the two rows want
opposite behaviour from the same instruction shape, and the honest resolution is to
make it a per-region mode rather than a global default.

  `maspsx=nopmarker`  ->  --honour-nop-marker   (default OFF)

VERIFIED BOTH WAYS:
  make check with the mode OFF   -> regions=510 AGREE, differing_bytes=0 MATCH, 253 tests OK
  0x80107C5C WITH the mode       -> 112 B, differing_bytes=0 MATCH
  0x80107C5C WITHOUT it          -> 108 B LENGTH-MISMATCH
So the default is the long-standing behaviour, and rows that need the marker opt in.

The tracked patch is regenerated and verified to reproduce both modified files exactly
from the pristine pinned checkout (tools/maspsx is git-ignored, so the patch is the only
reproducible carrier). docs/SETUP.md records the correction.

This is the third time this phase a worker found a defect in work the coordinator had
already shipped as verified -- the pattern is worth noting: a fix proven regression-free
against the CORPUS can still be wrong for an UNMATCHED row, because the corpus only
exercises the paths that already work.
This commit is contained in:
Christopher Williams
2026-09-24 09:15:47 -04:00
parent 55e59b3a4a
commit c4f4bc0cfa
3 changed files with 48 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/SETUP.md
+9 -1
View File
@@ -88,7 +88,15 @@ patch, and diffing the result against the working tree (byte-identical). It adds
after a branch/jump so GNU `as` can fill the slot.
- `--nop-on-reg-read` (region token `maspsx=regread`) — extend the load-delay predicate (cookbook
finding 27) so a following `jr`/`jalr` that *uses* the loaded register also gets the delay `nop`.
- **A default-on fix (no region token): maspsx now honours cc1's explicit `#nop` marker.** cc1 emits the
- **`nopmarker` (region token `maspsx=nopmarker`) — OPT-IN as of Phase 11.** cc1 emits an explicit
`#nop` marker when it thinks a load-delay nop is needed. For a bare-symbol store consumer
(`sw $2,D_801221C4`) the store's own `lui $at` expansion fills the delay slot, so the marker is
**spurious** and honouring it costs an instruction (worker C's `0x800AFDBC`: the original is
`lhu` / `lui at` / `sh` with NO nop). But `0x80107C5C` genuinely wants the nop (112 vs 108).
Phase 10 made the honouring unconditional and that was **too broad**; it is now per-region and
default OFF. Verified: `make check` green at 510 regions with it off, and `0x80107C5C` matches
112 B with the token and comes out 108 B LENGTH-MISMATCH without it.
- **A default-on fix (superseded by the opt-in above): maspsx now honours cc1's explicit `#nop` marker.** cc1 emits the
marker when it wants a load-delay nop; maspsx used to re-derive the need and could overrule cc1 for a
**bare-symbol store** consumer (`sw $2,D_801221C4`), because `uses_at` is True for a macro store while
`nop_at_expansion` is False for ASPSX >= 2.30, so neither test fired and the nop was dropped
tools/patches/maspsx-phase10-r1r2.patch
+31 -23
View File
@@ -39,16 +39,15 @@
def line_loads_from_reg(line: str, r_source: str, loads_to_reg=False) -> bool:
"""
NOTE: Returns True even if line might use $at expansion
@@ -448,6 +480,8 @@
gp_allow_la=False,
use_comm_section=False,
@@ -450,6 +482,7 @@
use_comm_for_lcomm=False,
+ no_jump_slot_nop=False,
+ nop_on_reg_read=False,
no_jump_slot_nop=False,
nop_on_reg_read=False,
+ honour_nop_marker=False,
):
self.lines = [x.strip() for x in lines]
@@ -460,6 +494,15 @@
@@ -462,6 +495,18 @@
self.nop_mflo_mfhi = nop_mflo_mfhi
self.nop_lw_lw = nop_lw_lw
@@ -60,11 +59,14 @@
+ # the loaded register as needing the delay nop (worker C's R2).
+ self.no_jump_slot_nop = no_jump_slot_nop
+ self.nop_on_reg_read = nop_on_reg_read
+ # Opt-in (Phase 11 correction): honour cc1's explicit `#nop` marker even when
+ # the following instruction's own macro expansion would fill the delay slot.
+ self.honour_nop_marker = honour_nop_marker
+
self.sltu_at = sltu_at
self.addiu_at = addiu_at
self.div_uses_tge = div_uses_tge
@@ -696,9 +739,30 @@
@@ -698,9 +743,34 @@
) -> List[str]:
res: List[str] = []
@@ -79,24 +81,28 @@
+ if reuse:
nop_required = False
+ # Phase 10 (developer-authorised): cc1 emits an explicit `#nop` marker
+ # when it wants a load-delay nop. maspsx used to re-derive the need and
+ # could overrule cc1: for a BARE-SYMBOL store consumer (`sw $2,D_801221C4`)
+ # `uses_at` is True (the store expands via $at) while `nop_at_expansion`
+ # is False for ASPSX >= 2.30, so neither test below fired and the nop was
+ # dropped -- losing an instruction cc1 had asked for (0x80107C5C,
+ # 0x8003A9C8). Honour the explicit marker instead of overruling it.
+ # Only the marker changes behaviour; every other path is untouched.
+ if self.get_next_instruction(
+ # Phase 10/11: cc1 emits an explicit `#nop` marker when it thinks a
+ # load-delay nop is needed. For a BARE-SYMBOL store consumer
+ # (`sw $2,D_801221C4`) `uses_at` is True (the store expands via $at) while
+ # `nop_at_expansion` is False for ASPSX >= 2.30, so neither test below
+ # fires and the marker is ignored -- which is the correct default, because
+ # the store's own `lui $at` expansion fills the delay slot (worker C's
+ # 0x800AFDBC: the original is `lhu` / `lui at` / `sh` with NO nop, and
+ # honouring the marker costs 2 instructions there).
+ #
+ # Phase 10 made this unconditional and it was TOO BROAD: 0x80107C5C DOES
+ # want the nop (108 vs 112). So it is opt-in per region now
+ # (`maspsx=nopmarker`). Default off; no registered region depends on
+ # either behaviour (the gate is green at 510 regions both ways).
+ if self.honour_nop_marker and self.get_next_instruction(
+ skip=0, ignore_nop=False, ignore_set=True, ignore_label=True
+ ) == "#nop":
+ reason = "cc1 emitted an explicit '#nop' marker"
+ nop_required = True
+
if not uses_at(next_instruction):
reason = f"'{next_instruction}' does not use $at"
nop_required = True
@@ -1102,7 +1166,7 @@
@@ -1105,7 +1175,7 @@
elif op in branch_mnemonics or op in jump_mnemonics:
res.append(line)
@@ -107,23 +113,25 @@
elif op == "move":
--- a/maspsx.py
+++ b/maspsx.py
@@ -62,6 +62,10 @@
@@ -62,8 +62,11 @@
parser.add_argument("--passthrough", action="store_true")
parser.add_argument("--use-comm-section", action="store_true")
parser.add_argument("--use-comm-for-lcomm", action="store_true")
+ # Phase 10 local additions (developer-authorised, opt-in; see
+ # tools/patches/maspsx-phase10-r1r2.patch and docs/SETUP.md).
+ parser.add_argument("--no-jump-slot-nop", action="store_true")
+ parser.add_argument("--nop-on-reg-read", action="store_true")
parser.add_argument("--no-jump-slot-nop", action="store_true")
parser.add_argument("--nop-on-reg-read", action="store_true")
+ parser.add_argument("--honour-nop-marker", action="store_true")
# decomp.me debugging
parser.add_argument("--print-output", action="store_true")
parser.add_argument("--print-input", action="store_true")
@@ -152,6 +156,8 @@
@@ -154,6 +157,9 @@
gp_allow_la=version_config.gp_allow_la,
use_comm_section=args.use_comm_section,
use_comm_for_lcomm=args.use_comm_for_lcomm,
+ no_jump_slot_nop=args.no_jump_slot_nop,
+ nop_on_reg_read=args.nop_on_reg_read,
+ honour_nop_marker=args.honour_nop_marker,
)
try:
tools/sf3_match
+8 -2
View File
@@ -216,7 +216,8 @@ _REGION_OPTION_KEYS = ("cc1", "as", "gp", "maspsx")
# `move`->`addu` rewrite and NO maspsx stage, letting `as` fill exactly the slots cc1
# left empty while leaving cc1's own `.set noreorder` windows alone.
_MASPSX_MODES = {"off": "--off", "noreordernop": "--no-jump-slot-nop",
"regread": "--nop-on-reg-read", "moves": "moves"}
"regread": "--nop-on-reg-read", "moves": "moves",
"nopmarker": "--honour-nop-marker"}
def parse_region_options(text: str, line_number: int) -> tuple[tuple[str, ...], tuple[str, ...], tuple[str, ...], bool, tuple[str, ...], bool]:
@@ -852,6 +853,10 @@ def add_toolchain_arguments(parser: argparse.ArgumentParser) -> None:
parser.add_argument("--no-jump-slot-nop", action="store_true",
help="maspsx mode: suppress the unconditional reorder nop after a "
"branch/jump so GNU as can fill the slot (region: maspsx=noreordernop)")
parser.add_argument("--honour-nop-marker", action="store_true",
help="maspsx mode: honour cc1's explicit `#nop` marker even when the "
"following macro expansion would fill the delay slot "
"(region token: maspsx=nopmarker)")
parser.add_argument("--maspsx-moves", action="store_true",
help="run the `move`->`addu` rewrite and NO maspsx stage, so GNU as "
"reorder mode fills the slots (region token: maspsx=moves)")
@@ -963,7 +968,8 @@ def resolve_toolchain(args: argparse.Namespace) -> Toolchain:
maspsx_flags=tuple(
flag for flag, enabled in (
("--no-jump-slot-nop", getattr(args, "no_jump_slot_nop", False)),
("--nop-on-reg-read", getattr(args, "nop_on_reg_read", False))) if enabled),
("--nop-on-reg-read", getattr(args, "nop_on_reg_read", False)),
("--honour-nop-marker", getattr(args, "honour_nop_marker", False))) if enabled),
aspsx_version=args.aspsx_version,
)