fix(vu1): maintain MAC/STATUS/CLIP flag registers, correct the flag-reading lower-op table, and saturate FTOI - #189
Draft
smmathews wants to merge 1 commit into
Draft
Conversation
…eading lower-op table, and saturate FTOI The VU1 interpreter declared mac/status/clip fields and the lower-op instructions that read them, but the upper FMAC pipeline never wrote mac, and STATUS was written by exactly one instruction using an unsourced immediate extraction. Every arithmetic FMAC vf-writer and ACC-writer now computes MAC over its DEST lanes and folds it into a STATUS live/sticky pair; MAX/MINI (which the PS2 FMAC pipeline runs without any flag update, matching PCSX2's applyMinMax) and the nine ITOF/FTOI/ABS unary ops and the plain dest-mask lower ops (LQ, MOVE, MR32, LQI, LQD, MFIR, MFP) are explicitly excluded from the flag path. Separately, the flag-reading lower-op table at 0x18-0x1C had two of its four case bodies swapped (0x18/0x1A) and was missing FMOR (0x1B) and FCGET (0x1C, which ran a duplicate FMOR body instead). FSSET's immediate extraction is replaced with the formula sourced from PCSX2's reference VU interpreter, since none of the formulas previously in circulation for this codebase matched it. CLIP's shift-register accumulation is now masked to 24 bits, since without it FCOR permanently loses the ability to compare true once five or more CLIP instructions have run. FTOI0/ 4/12/15 now saturate on overflow instead of relying on undefined behavior, which on x86 silently produced INT_MIN for large positive values too. All of this is sourced against PCSX2 pcsx2/VUops.cpp and pcsx2/VUflags.cpp (see EVIDENCE.md); pipeline latency (FDIV/Q, EFU/P, and any multi-cycle flag commit delay) is out of scope here and left for a follow-up with its own citation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The VU1 interpreter declares a flag model it does not implement.
VU1Statehas hadmac,status, andclipfields since it was introduced, and the lower-op instructions that readthem (FMAND, FMEQ, FMOR, FSEQ, FSAND, FSOR, FCEQ, FCAND, FCOR) have been present the whole
time, but nothing in the upper (FMAC) pipeline ever wrote
mac, and STATUS was written byexactly one instruction with an unsourced immediate extraction. Separately, the lower-op
table that FMEQ/FMAND/FMOR/FCGET live in has two of its four case bodies swapped, is missing
the fourth op entirely, and FTOI has no saturation, so out-of-range conversions are
undefined behavior.
This PR:
and ACC-writers), gated per DEST lane, and folds it into a STATUS live/sticky pair.
MAX/MINI, which run in the FMAC pipeline but raise no flags on hardware, are excluded.
0x1C is FCGET (previously absent; the slot instead ran the FMOR (OR) body, which belongs
at 0x1B).
Everything here is a flag/value fix with an immediate-commit model. This PR does not model
any pipeline latency (the FDIV/Q delay, the EFU/P delay, or a multi-cycle flag/CLIP commit
delay). Those are real and worth a follow-up, but they are unsourced by this project today
and belong in their own PR with their own citation, not folded into a value-correctness fix.
The MAC register was declared but never written
returns three reads (FMAND, FMEQ, FMOR before this fix) and zero writes anywhere in the
upper pipeline. Every FMAC instruction computed a result and wrote it through
applyDeststraight to the destination register; nothing ever touched
m_state.mac. FMAND/FMEQ/FMORtherefore always evaluated the flag register as zero.
The write path split: flag computation is a per-call-site property, not a mode flag
The previous shape for a runtime flag pipeline would be a mutable "am I inside an upper op"
flag. That is a state machine hiding a compile-time fact: whether an instruction computes
flags is fixed by which instruction it is, not by when it runs. So the destination-mask copy
(
applyDest, renamedwriteDestMasked) and the flag computation (computeFmacFlags) arenow two separate calls, and which call sites include the flag call is visible with a plain
grep:
OPMSUB): both calls.
writeDestMaskedonly. They run in the FMACpipeline but leave MAC and STATUS unchanged on hardware (PCSX2 routes them through
applyMinMax, which performs no flag update), so they are excluded exactly like the
unary ops below. A dedicated negative-control test runs a MAX between a producing FMAC
and a flag read, and asserts MAC is unchanged by it.
applyDestAcc'sbody now computes flags before writing to
m_state.acc. MAC reflects the FMAC resultwhether the destination is a vf register or the accumulator.
MFIR, MFP):
writeDestMaskedonly. These are not FMAC ops and must not touch MAC. Adedicated negative-control test runs all three of ITOF0, FTOI0, and ABS between a
producing FMAC and a flag read, and asserts MAC is unchanged by them.
The ACC-writer group is uniform (every ACC-writer raises flags, so
applyDestAcccanbundle both calls unconditionally); the vf-writer group is mixed (MAX/MINI and the unary
ops do not raise flags, arithmetic vf-writers do). Keeping the flag call as a separate line
at each vf-writer site, rather than bundling it into a single vf-write helper, is what makes
that per-op membership visible at the point of use instead of hidden behind a shared wrapper.
MAC and STATUS bit layout
MAC is 16 bits:
Z[3:0],S[7:4],U[11:8],O[15:12], each nibble ordered x,y,z,w fromthe high bit to the low bit. Z and U are not mutually exclusive: Z fires on a +/-0.0 result
or on a denormal result (a denormal is an underflow that PCSX2 flushes to zero, so it raises
both U and Z together, matching PCSX2's
0x0101<<shift); U fires when the result's exponentfield is 0 with a nonzero mantissa (denormal); O fires when the exponent field is all ones
(inf or NaN; this project does not distinguish them, matching the sourced reference). S
mirrors the IEEE754 sign bit unconditionally. Lanes outside DEST are not evaluated and read 0
in every group.
STATUS is modelled as a 12-bit register: a live half (bits 5:0) that is replaced whole by
every FMAC instruction, and a sticky half (bits 11:6) that only ever accumulates (bitwise
OR) so FSAND/FSOR can answer "has this ever happened". Each half only carries four of its
six bits (Z, S, U, O); the I (invalid) and D (divide-by-zero-adjacent) bits that real
hardware also tracks in STATUS are not modelled here and are left zero. This PR does not
claim to model them; a future PR that adds them can do so without touching this shape.
FSSET's immediate extraction was sourced from PCSX2 and does not match any of the three
formulas that had been circulating for this codebase. It writes only the sticky half and
leaves the live half exactly as the most recent FMAC left it.
The lower-op table was transposed, not just mislabeled
Before this fix, case 0x18 was labeled FMAND but its body computed AND at the wrong opcode
slot (FMAND is really at 0x1A); case 0x1A was labeled FMEQ but its body computed EQ, also at
the wrong slot (FMEQ is really at 0x18); case 0x1C was labeled FMOR and computed OR, but
0x1C is FCGET and FMOR belongs at 0x1B, which had no case at all. Sourced against the
reference VU interpreter's lower-op dispatch table: 0x18=FMEQ, 0x19 unassigned, 0x1A=FMAND,
0x1B=FMOR, 0x1C=FCGET (reads CLIP's low 12 bits into the destination register, guarded on a
nonzero destination). Each op's body implements the op its old comment named; the bug was
that the comment and the opcode slot disagreed with the reference table.
CLIP's shift-register accumulation was never masked
m_state.clip = (m_state.clip << 6) | flags;accumulates 6 new bits every CLIP instructionwith no mask. FCSET does mask its own write to 24 bits, so it is not accurate to say CLIP is
never masked; the precise claim is that the shift-register accumulation is unmasked. Once
five or more CLIP instructions have run, bits above bit 23 are permanently set and FCOR's
== 0xFFFFFFcomparison can never go true again, because CLIP's actual value now alwaysexceeds what a 24-bit compare can match, regardless of what the low 24 bits are. This PR
adds
& 0xFFFFFFuafter the shift, matching the reference implementation's own mask at thesame point. A test runs five CLIPs that would overflow the 24-bit window, then confirms FCOR
can go true again afterward.
FTOI had no saturation, and the plain cast was undefined behavior
FTOI0/4/12/15 cast a (possibly pre-scaled) float straight to
int32_twithstatic_cast<int32_t>. Casting a float that does not fit in the target integer type isundefined behavior in C++; in practice, on x86 this compiles to
cvttss2si, whosedocumented behavior on an out-of-range or NaN input is to return the "integer indefinite"
value
0x80000000, regardless of the input's sign. That means a large positiveout-of-range float silently produced
INT_MIN, notINT_MAX, pre-fix; only thealready-correct-by-coincidence negative case matched what saturation should produce. This PR
adds explicit saturation: a scaled magnitude of 2^31 or more (checked on the exponent field,
covering inf and NaN) clamps to
0x7fffffffor0x80000000by sign bit, otherwise theexisting cast is used unchanged.
The decode cache is unaffected
#158added a decode cache keyed onPS2Memory::getVU1CodeGeneration(). It caches thedecode of an instruction pair (opcode fields, iBit/eBit, execution order), not any
execution-time result, so none of this PR's flag state interacts with it: two runs of the
same cached instruction still compute fresh MAC/STATUS/CLIP values from the current
operands every time.
Reproduction
The pre-fix defects can be reproduced directly against a commit before this one:
Full sourcing and the one-line-mutation proof for every pinned property are in EVIDENCE.md.