Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DSP LLE JIT: Fix Update_SR_Register16_OverS32 #10709

Merged
merged 1 commit into from Jun 3, 2022

Conversation

Pokechu22
Copy link
Contributor

There were 3 bugs here:

  • The input register for the full register wasn't actually being used; it was read into RCX but RCX wasn't used by Update_SR_Register16_OverS32 (except as a scratch register). The way the DSP LLE recompiler uses registers is in general confusing, so this commit changes a few uses to have a variable for the register being used, to make code a bit more readable. (Default parameter values were also removed so that they needed to be explicitly specified).
  • Update_SR_Register16 was doing a 64-bit test, when it should have been doing a 16-bit test. For the most part this doesn't matter due to sign-extension, but it does come up with e.g. ORI or ANDI. This change might fix bugs in official uCode, but it's unlikely.
  • Update_SR_Register16_OverS32 did the over s32 check, and then called Update_SR_Register16. Update_SR_Register16 masks $sr with ~SR_CMP_MASK, clearing the over s32 bit. Now the over s32 check is performed after calling Update_SR_Register16 (without masking a second time). No official uCode cares about the over s32 bit.

Here's my hardware tests, along with its output on real hardware, the DSP interpreter, and the recompiler before and after this change (as bin and txt from DSPTool -p): 40bit_ins_test.zip. This test wasn't intended to find this bug, and instead will be included in a separate PR.

There were 3 bugs here:

- The input register for the full register wasn't actually being used; it was read into RCX but RCX wasn't used by Update_SR_Register16_OverS32 (except as a scratch register).  The way the DSP LLE recompiler uses registers is in general confusing, so this commit changes a few uses to have a variable for the register being used, to make code a bit more readable.  (Default parameter values were also removed so that they needed to be explicitly specified).
- Update_SR_Register16 was doing a 64-bit test, when it should have been doing a 16-bit test.  For the most part this doesn't matter due to sign-extension, but it does come up with e.g. `ORI` or `ANDI`.
- Update_SR_Register16_OverS32 did the over s32 check, and then called Update_SR_Register16.  Update_SR_Register16 masks $sr with ~SR_CMP_MASK, clearing the over s32 bit.  Now the over s32 check is performed after calling Update_SR_Register16 (without masking a second time).  No official uCode cares about the over s32 bit.
@lioncash lioncash merged commit c8ab236 into dolphin-emu:master Jun 3, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants