Ryujinx icon indicating copy to clipboard operation
Ryujinx copied to clipboard

Implement some missing 32-bit instructions (UHADD8, VCVT, VRINTR, VMLAL, VPADAL, VPADDL, VQADD, VQMOVN, VQMOVUN, VQSUB)

Open piyachetk opened this issue 3 years ago • 11 comments

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.

piyachetk avatar Apr 24 '21 11:04 piyachetk

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.

piyachetk avatar May 02 '21 09:05 piyachetk

I've added the VMLAL (integer), VPADDL, VPADAL, VQADD, and VQSUB instructions along with their test units.

piyachetk avatar May 28 '21 12:05 piyachetk

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.

CeruleanSky avatar May 29 '21 19:05 CeruleanSky

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

EmulationFanatic avatar Jun 03 '21 00:06 EmulationFanatic

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.

piyachetk avatar Jun 03 '21 07:06 piyachetk

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.

riperiperi avatar Jun 29 '21 22:06 riperiperi

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.

piyachetk avatar Jun 30 '21 01:06 piyachetk

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.

CeruleanSky avatar Aug 31 '21 16:08 CeruleanSky

I'm okay with that idea. Which instruction should be split into its own PR? Is it only UHADD8?

piyachetk avatar Aug 31 '21 16:08 piyachetk

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.

CeruleanSky avatar Oct 12 '21 23:10 CeruleanSky

needs a rebase

ScottNash042 avatar Jan 14 '24 12:01 ScottNash042

Latest missing instructions are ported in #6185, this can be closed.

AcK77 avatar Jan 27 '24 01:01 AcK77