Enhance DSPAssemblyTest, and fix various DSPTool bugs discovered while doing so #10750
+177
−74
Conversation
This file contains 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
These were moved into UnitTests in dolphin-emu#5449.
Before, the file just existed as the source code for HermesBinary.cpp, but we can test that things assemble correctly too (compare DSPTestBinary.cpp and DSPTestText.cpp). A bit of jank is needed due to MSVC limitations (see https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2026?view=msvc-170).
…ruction with a large immediate For instance, ending with 0x009e (which you can do with CW 0x009e) indicates a LRI $ac0.m instruction, but there is no immediate value to load, so before whatever garbage in memory existed after the end of the file was used. The bounds-checking also previously assumed that IRAM or IROM was being used, both of which were exactly 0x1000 long.
It's included in the section before, so it's helpful to supply here too.
…he expected result This reveals that both HermesText and HermesBinary fail. HermesBinary would have failed on master, too, if this had been implemented.
Before, both 1441 and 147f would disassemble as `lsr $acc0, dolphin-emu#1`, when the second should be `lsr $acc0, #-1`, and both 14c1 and 14ff would be `asr $acc0, dolphin-emu#1` when the second should be `asr $acc0, #-1`. I'm not entirely sure whether the minus signs actually make sense here, but this change is consistent with the assembler so that's an improvement at least. devkitPro previously changed the formatting to not require negative signs for lsr and asr; this is probably something we should do in the future: devkitPro/gamecube-tools@8a65c85 This fixes the HermesText and HermesBinary tests (HermesText already wrote `lsr $ACC0, #-5`, so this is consistent with what it used before.)
We already have the data for this, so this seems like a useful thing to do. However, neither of the new tests currently pass...
This change makes assembling DSPTestText match DSPTestBinary, though HermesText doesn't yet match HermesBinary. The test data was originally added on April 18, 2009 in e7e4ef4. Then, set16 and set40 were swapped on April 22, 2009 89178f4, which updated the DSPSpy version of dsp_code, but not the version in DSPTool used for testing. So, when the test was made, the assembled data matched the text, but a few days after it no longer did. Similarly, on Jul 7, 2009 in 1654c58 the conditional instructions were adjusted, and 0x1706 was changed from JRL to JRNC and 0x0297 was changed from JGE to JC. For what it's worth, devkitPro made the same changes on May 31, 2010 in devkitPro/gamecube-tools@8a65c85 and updated their version of the asnd ucode (which is this ucode) on June 11, 2011 in devkitPro/libogc@b1b8eca (though this update also includes other feature changes). Note that at the time, they didn't reassemble the ucode unless they made changes to it; the assembled was stored in the repo until devkitPro/libogc@bfb705f~...d20f9bd. This fixes the following failures with Hermes: !! 0015 : 8e00 vs 8f00 - set16 vs set40 !! 016f : 8e00 vs 8f00 - set16 vs set40 and with Hermes: !! 0014 : 8e00 vs 8f00 - set16 vs set40 !! 0063 : 8e00 vs 8f00 - set16 vs set40 !! 019b : 1706 vs 1701 - jrnc $AR0 vs jrl $AR0 !! 01bf : 0297 vs 0290 - jc 0x01dc vs jge 0x01dc !! 01d2 : 0297 vs 0290 - jc 0x01dc vs jge 0x01dc Hermes has the remaining failures: !! 027b : 03c0 vs 03a0 - andcf $AC1.M, #0x8000 vs andf $AC1.M, #0x8000 !! 027d : 029d vs 0294 - jlz 0x027a vs jnz 0x027a
I don't know what happened here, unfortunately. The version of dsp_mixer.s added to libogc on Nov 14, 2008 in devkitPro/libogc@c76d8b8 uses andcf and jlz here, and the version we have matches the one from Feb 5, 2009 in devkitPro/libogc@ae5c3a5 exactly (prior to the fixes in my previous commit). I can't see any reason why wait_for_dsp_mail would be changed like this. ANDCF and ANDF were previously swapped and JNE/JEQ/JZR/JNZ became JNZ/JZ/JLNZ/JLZ on Apr 3, 2009 in 7c4e654, corresponding to a change Hermes made on Nov 10, 2008 in devkitPro/gamecube-tools@2cea6d9. But these predate the test being added. The only other information I can find is that ASNDLIB 1.0 released on November 11, 2008, at https://web.archive.org/web/20120326145022/http://www.entuwii.net/foro/viewtopic.php?f=6&t=87 (but there aren't any surviving links from there).
This is an assembled version of HermesText.cpp, so the same license applies to it.
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
Rumi-Larry
reviewed
Jun 15, 2022
| // Thanks to Duddie for you hard work and documentation | ||
| Copyright (c) 2008 Hermes <www.entuwii.net> |
There was a problem hiding this comment.
Indeed it is, but it's the one you'll find in the actual uCode: https://github.com/devkitPro/libogc/blob/6c104219be4f3741c59446948052311a405de7cf/libasnd/dsp_mixer/dsp_mixer.s#L5 / https://github.com/Pokechu22/dolphin/blob/dec48ed7de1e9710dc2aad5690e3a32b8ee8ed13/Source/UnitTests/Core/DSP/HermesText.cpp#L1-L10 and I'm not sure if there's a better choice available for attribution.
The website itself has some archive coverage at https://web.archive.org/web/20090415012321/http://www.entuwii.net/foro/ (but it's mostly surface-level unfortunately).
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.
No description provided.