Ryujinx
Ryujinx copied to clipboard
Implement some missing 32-bit instructions (UHADD8, VCVT, VRINTR, VMLAL, VPADAL, VPADDL, VQADD, VQMOVN, VQMOVUN, VQSUB)
Implemented instructions
- UHADD8
- VCVT (between floating-point and fixed-point)
- VMLAL (integer)
- VPADAL
- VPADDL
- VQADD
- VQMOVN
- VQMOVUN
- VQSUB
- VRINTR
Fixes #1344 Fixes #1644 Fixes #1781 Fixes #1811
Ryujinx/Ryujinx-Games-List#145
- This game will now run (with audio issues but it's playable)
Ryujinx/Ryujinx-Games-List#3506
- This game won't crash but it will show some weird artifacts (unplayable)
Ryujinx/Ryujinx-Games-List#2998 Ryujinx/Ryujinx-Games-List#1951
- These games need the VQMOVN/VQMOVUN instruction and should now be fixed but I haven't tested them yet. Unless they also require some other missing 32-bit instructions, they should be able to boot just fine.
Ryujinx/Ryujinx-Games-List#1943
- This game needs VQMOVN/VQMOVUN, VQSUB, and VMLAL. It still crashes as it need the VQDMULH instruction.
Download the artifacts for this pull request:
I've added some unit tests for both instructions. VQMOVN/VQMOVUN seems to be doing fine. VCVT seems to have some problems with rounding (not sure why).
Edit: It passes the tests now.
I've added the VMLAL (integer), VPADDL, VPADAL, VQADD, and VQSUB instructions along with their test units.
No More Heroes (Ryujinx-Games-List #2809) requires UHADD8 and now boots to the point where it speaks the word Marvelous before crashing due to what looks like memory access errors.
00:00:28.952 |E| HLE.GuestThread.15 Cpu InvalidAccessHandler: Invalid memory access at virtual address 0x0000000000000000.
No logs provided as I am just confirming that with the new instruction the game goes further.
The incorrect audio in LIMBO leads me to believe there is a problem with the implementation. In past CPU instruction PRs, it was an indicator that there was something wrong; in the case of this PR submitted by @saldabain, the author had to make adjustments in order to correct bad audio in a handful of games that had their missing opcodes fulfilled. If you are on the Ryujinx Discord server you can take a look at the change below.
https://discord.com/channels/410208534861447168/410432399500115968/784151448728240129
AFAIK, Limbo only uses VCVT and VQMOVN instructions. Both of my implementations pass the unit tests just fine. I also thought about FPSCR rounding mode and cumulative saturation bit but this VCVT variant does not use the default rounding mode from FPSCR and my VQMOVN implementation uses existing fallback methods which are also used in USAT instructions which do set the cumulative saturation bit (and they all pass the tests). My unit tests can be wrong though but I don't think it is.
Did the issues fixed in your recent commits appear in the tests, or in any of the affected games? I don't have any of them personally.
I've only personally tested two games. Limbo still has audio issues and Planescape crashes due to missing another instruction. The Star Wars game was tested by a friend of mine so I don't know how the latest commit is doing in that game.
I'm hoping this pull request wasn't forgotten and is still being considered for review as at least on the issue tracker it hasn't really had activity for a couple months. If the issue is instruction review takes a while, perhaps it could be split and some of the instructions(UHADD8 maybe as No More Heroes 1&2 use it) be put into new pull requests as the instructions are needed for several games, and it would be nice to use master so others can easily check if some games go further or are still crashing in regards to their other aspects.
I'm okay with that idea. Which instruction should be split into its own PR? Is it only UHADD8?
I only mentioned breaking up the PR because gdkchan said "Sorry for the delay, those instruction implementations are more complicated to review as we need to compare the implementation against the reference on the manual to ensure they are correct. "and it has been over 100 days now since the maintainers have looked at this.
Maybe divided up it could be reviewed on the order of weeks instead of months.
needs a rebase
Latest missing instructions are ported in #6185, this can be closed.