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

Fixed missing and incorrectly implemented RSP opcodes #10

Merged
merged 17 commits into from
Mar 22, 2024

Conversation

mpharoah
Copy link
Contributor

@mpharoah mpharoah commented Jan 15, 2024

Added all the missing RSP opcodes and fixed some that were broken. With these changes (alongside a few mupen core side changes), parallel-rsp now passes 99.67% of the RSP tests in n64-systemtest. The only remaining failing test is the one that tests that the RSP and CPU can run in parallel.

If you want to test out the changes with the mupen side changes as well, you can use this fork of parallel-n64 which includes the changes in this PR as well as the mupen core rsp changes: https://gitlab.com/parallel-launcher/parallel-n64/-/tree/v2.14-8?ref_type=tags (builds here: https://gitlab.com/parallel-launcher/parallel-n64/-/releases/v2.14-8)

Tested the changes using a build of n64-systemtest that only runs the startup/teardown test and all the RSP related tests. Also ran some roms using different microcodes as a regression test on actual games.

Full list of changes:

  • Fixed an RSP hang when setting HALT but not BREAK on the status register
  • Fixed the SRAV instruction being able to write to the R0 register
  • Implemented missing instructions: LWU, VMACQ, VMULQ, VRNDP, VRNDN
  • Fixed the SLTIU instruction incorrectly interpreting the immediate value as unsigned (it still reads it as signed, but then does an unsigned comparison)
  • Fixed the VRCP* and VRSQ* families of instructions not handling the case where e > 7 correctly
  • Fixed the VMOV instruction not setting the accumulator correctly
  • Fixed only one of the two VNULL opcodes being handled
  • Fixed the behaviour of invalid/reserved opcodes (wasn't setting the accumulator like it does on console)
  • Fixed behaviour when both setting and clearing the same STATUS bit at the same time (no effect)
  • Fixed semaphore register not being set back to 1 on a read from the RSP
  • [with PARALLEL_INTEGRATION enabled] Increment the timeout when reading a non-zero value from the semaphore register to ensure it doesn't hang if the RSP spin locks waiting for the CPU to set it.

@mpharoah mpharoah closed this Jan 15, 2024
@mpharoah mpharoah deleted the mpharoah/rsp-accuracy branch January 15, 2024 22:34
@mpharoah mpharoah restored the mpharoah/rsp-accuracy branch January 15, 2024 22:36
@mpharoah mpharoah reopened this Jan 15, 2024
@mpharoah
Copy link
Contributor Author

I don't know why this PR is also including changes from my previous PR too.

@LibretroAdmin
Copy link
Collaborator

Would this be safe to merge as-is?

BTW, we are trying to bring the parallel n64 core on the buildbot inline with your downstream. @warmenhoven might have some questions about the Apple side regarding Mac/iOS though. We think it was mentioned on your downstream that Apple works fine with all the changes, however we have experienced initial crashes on launch. Would you perhaps be willing to talk to warmenhoven and we might figure out some solution? We think it would be in everyone's best interests if the upstream core has all the benefits of the downstream as well considering all the great improvements that have been made in it. Thanks in advance.

You can see our current PR here -
libretro/parallel-n64#773

@mpharoah
Copy link
Contributor Author

mpharoah commented Jan 24, 2024

You would be best to talk with aglab2 about the MacOS side of things, as I don't have much knowledge or experience in the Apple side of things, and my Mac virtual machine isn't capable of running RetroArch. I'm more than happy to help with any Linux or non-OS-specific things though.

I believe this should be safe to merge as-is and use with the existing core (as in it shouldn't freeze or break on anything that wasn't already broken), but it should really include these core changes (the 4 commits starting with the one on January 8) in rsp_core.c in the core repo as well to ensure everything runs correctly:
https://gitlab.com/parallel-launcher/parallel-n64/-/commits/master/mupen64plus-core/src/rsp/rsp_core.c

@LibretroAdmin
Copy link
Collaborator

Hi there, do you happen to be in contact with this person aglab2? How could we reach them?

@LibretroAdmin
Copy link
Collaborator

These changes to rsp_core.c, are they already incorporated in mupen64plus-core upstream repo or are they not needed there? Since this codebase (parallel-rsp) gets used both by parallel-n64 and mupen64plus (both next and upstream variants)

@mpharoah
Copy link
Contributor Author

@aglab2

@mpharoah
Copy link
Contributor Author

It looks like mupen64plus-libretro-nx might need changes to its equivalent file as well to pass all RSP tests

@mpharoah
Copy link
Contributor Author

Yeah, mupen still freezes when running the RSP tests (this isn't new behaviour caused by these changes- its existing behaviour that isn't fixed due to missing the core side changes)

@m4xw
Copy link
Collaborator

m4xw commented Jan 24, 2024

its all on the system test branch tho its old by now.
I worked through a bunch of it over a year ago but its been sitting there

@aglab2
Copy link

aglab2 commented Jan 25, 2024

I think this PR should be safe to merge, i cannot imagine how it will cause issues on macOS. If needed, i can debug. Regarding libretro/parallel-n64#773 we should discuss it there or better on discord (my handle is aglab2) as there can be tons of various issues. Notable ARM64 dynarec crashes on some games https://gitlab.com/parallel-launcher/parallel-n64/-/issues/8 and I did take a look at them yet.

@LibretroAdmin
Copy link
Collaborator

Hi @aglab2 , thanks for stopping by. How can we reach you on Discord? Can you come to our libretro discord or do we have to go to yours? Either is fine with us.

@m4xw
Copy link
Collaborator

m4xw commented Jan 25, 2024

Generally i dont see issues with this PR but i would prefer if we confirm that some of these changes properly autovectorize

@mpharoah
Copy link
Contributor Author

What do you mean autovectorize?

@m4xw
Copy link
Collaborator

m4xw commented Jan 30, 2024

What do you mean autovectorize?

Some of the loops you introduced look to me like the compiler wont be able to apply some optimizations on it, would need to check it with verbose vectorization and see if it complains

@m4xw
Copy link
Collaborator

m4xw commented Mar 22, 2024

Gonna do a preliminary merge with potential fixup afterwards as i integrate it back into nx

@m4xw m4xw merged commit 705f4c2 into libretro:master Mar 22, 2024
@mpharoah mpharoah deleted the mpharoah/rsp-accuracy branch March 22, 2024 15:40
@Rosalie241
Copy link
Contributor

@mpharoah commit c285e65 introduces a regression with Banjo Tooie, it makes the camera placement incorrect, see Rosalie241/RMG#242 for further details, can you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants