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 masking and sign extension fixes #10032

Merged
merged 4 commits into from Aug 18, 2021
Merged

Conversation

Pokechu22
Copy link
Contributor

The cr and prod.h registers are 8 bits and do not experience sign extension. Also, the SR_100 bit always seems to read back as 0, regardless of what's written to it sr. Lastly, ac0.h and ac1.h do experience sign extension; this is already implemented, but the implementation worked incorrectly on the interpreter when the whole acc0 or acc1 register was updated and then ac0.h or ac1.h were read back directly.

Results

Apologies for how unreadable this is. Think of it more as a table of thumbnails than something you can directly compare.

Test Hardware Old JIT Old Int New JIT New Int
Less Screenshot 2021-08-15 19-53-12 OHBCHB_2021-08-15_19-55-44_Old_JIT OHBCHB_2021-08-15_19-56-50_Old_Int OHBCHB_2021-08-15_19-59-04_New_JIT OHBCHB_2021-08-15_19-59-18_New_Int
FFFF Screenshot 2021-08-15 22-10-38_FFFF OHBCHB_2021-08-15_21-43-23_Old_JIT_FFFF OHBCHB_2021-08-15_21-45-24_Old_Int_FFFF OHBCHB_2021-08-15_21-47-10_New_JIT_FFFF OHBCHB_2021-08-15_21-49-24_New_Int_FFFF
0000 Screenshot 2021-08-15 22-11-12_0000 OHBCHB_2021-08-15_21-43-43_Old_JIT_0000 OHBCHB_2021-08-15_21-45-26_Old_Int_0000 OHBCHB_2021-08-15_21-47-12_New_JIT_0000 OHBCHB_2021-08-15_21-49-28_New_Int_0000
007F Screenshot 2021-08-15 22-11-26_007F OHBCHB_2021-08-15_21-43-47_Old_JIT_007F OHBCHB_2021-08-15_21-45-29_Old_Int_007F OHBCHB_2021-08-15_21-47-14_New_JIT_007F OHBCHB_2021-08-15_21-49-31_New_Int_007F
0080 Screenshot 2021-08-15 22-11-42_0080 OHBCHB_2021-08-15_21-43-49_Old_JIT_0080 OHBCHB_2021-08-15_21-45-31_Old_Int_0080 OHBCHB_2021-08-15_21-47-16_New_JIT_0080 OHBCHB_2021-08-15_21-49-33_New_Int_0080
0100 Screenshot 2021-08-15 22-11-56_0100 OHBCHB_2021-08-15_21-43-52_Old_JIT_0100 OHBCHB_2021-08-15_21-45-32_Old_Int_0100 OHBCHB_2021-08-15_21-47-18_New_JIT_0100 OHBCHB_2021-08-15_21-49-36_New_Int_0100

For the less test, the JIT behaves the same as it did before and as it does now, as it handled sign extension properly. Its result is not correct for the sr register, but that's a separate issue (the JIT seems to not update flags for overflow properly) which will be handled later. The interpreter now exactly matches hardware.

For the other tests, the old JIT and old interpreter had the same incorrect behavior, and the new JIT and the new interpreter have the same behavior that matches hardware.

I'm not completely confident on the code for the JIT, which I based based on the code for DSP_REG_ACH0. It seems to work, but the TODO about immediate values is a bit odd.

@shuffle2
Copy link
Contributor

kinda unrelated, but have you seen https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/Core/DSP/Interpreter/DSPIntMultiplier.cpp#L104 / know example that triggers the error the comment is talking about? Wanted to check it out before (but don't have hw to test on... :) )

@Pokechu22
Copy link
Contributor Author

I don't, but I'm guessing it's similar to the rounding that happens with e.g. clrl (which seems to be round-to-even), probably complicated by the fact that there are two middle product registers (and Dolphin doesn't currently emulate the way the two product registers are written during multiplication; I don't think it's been documented anywhere and I haven't looked into how it works).

Copy link
Member

@lioncash lioncash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing what @Tilka said on IRC in that we really should be making the tests for the unit-testing system that runs for every PR instead of for one that needs to manually be run

Source/Core/Core/DSP/Interpreter/DSPInterpreter.cpp Outdated Show resolved Hide resolved
Source/Core/Core/DSP/Interpreter/DSPInterpreter.cpp Outdated Show resolved Hide resolved
@Pokechu22 Pokechu22 force-pushed the dsp-lle-masks branch 2 times, most recently from 5af1270 to e17cda9 Compare August 16, 2021 20:48
The extension needs to happen in SetLongAcc, not GetLongAcc, as the extension needs to always be reflected in acS.h.

There is no functional difference with the write handler for acS.h, but it is more readable than 4 casts in a row.
m_gpr.WriteReg calls PutReg which already handles the sign extension.
@Tilka Tilka merged commit 3aaab25 into dolphin-emu:master Aug 18, 2021
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants