Repository navigation
VU1: JR and JALR read the current VI, not the branch backup - #418
Open
NoahBPeterson wants to merge 1 commit into
Open
NoahBPeterson wants to merge 1 commit into
NoahBPeterson wants to merge 1 commit into
Conversation
JR and JALR used the branch backup of a VI written by the previous instruction, like the conditional branches. Only the conditional branches take that backup; JR/JALR read the register as it is now (PCSX2's VU interpreter and microVU). Computed jumps such as `ISUB vi9, vi9, vi8; JR vi9` into an unrolled loop's remainder went to the wrong instruction. Found by a differential run of recorded VU1 programs (from True Crime: New York City) against PCSX2's VU1 interpreter; with the fix every recorded run matches on that branch. Test: JR right after an IADDIU to its register branches to the new value. (ported from commits d3953af and 8cfede8 on the author's tcnyc branch; the PCSX2 differential tool itself stays there, as it builds against a PCSX2 source tree) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
I'm Claude, opening this on behalf of @NoahBPeterson. We're working on a static recomp of True Crime: New York City with PS2Recomp, and this is one of the bugs we found along the way.
What's wrong
When an instruction writes a VI register, the next conditional branch (IBEQ, IBNE, IBLTZ, ...) reads the old value:
readBranchVireturns the backup. JR and JALR use the same backup, but on the VU they don't. They read the register as it is now. PCSX2's VU interpreter and microVU both do this.The difference shows up in computed jumps, e.g.
ISUB vi9, vi9, vi8followed byJR vi9into the remainder of an unrolled loop: the jump went to the old address.What changed
ps2_vu1_lower.cpp: JR and JALR readm_state.vi[is]directly instead ofreadBranchVi(is)(2 lines plus a comment).ps2_vu1_tests.cpp: the existing test "JR shares the one-instruction VI branch visibility rule" asserted the old behaviour. It now checks that JR right after an IADDIU to its register branches to the new value.Testing
ps2x_testsonmain(e563698) plus this change: 446/446 on macOS arm64 (Apple clang).VIS->viSource) but keepsreadBranchVi. Whichever merges second needs a trivial rebase.🤖 Generated with Claude Code