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

Propertly Truncate Paired Single Move Instructions #13059

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Geotale
Copy link
Contributor

@Geotale Geotale commented Sep 8, 2024

Matches hardware in rounding paired singles after move operations!!
Move operations consist of operations which only transfer direct bits (including absolute values and merges)
This also accounts for ps_rsqrte because it has a similar quirk
Specifically in hardware they're rounded in accordance to their slot:

  • PS0 rounds only the mantissa in accordance to the set rounding mode
  • PS1 truncates the mantissa

ps_rsqrte also only truncates for PS0 for some reason ^^;
This has all been tested on hardware, along with a few edge case tests to ensure this implementation works

Matches hardware in rounding paired singles after move operations!!
Move operations consist of operations which only transfer direct bits
This also accounts for ps_rsqrte, because it has a similar quirk
Specifically in hardware they're rounded in accordance to their slot:
- PS0 rounds *only the mantissa* in accordance to the set rounding mode
- PS1 truncates the mantissa
ps_rsqrte also truncates for PS0 for some reason ^^;
This has all been tested on hardware, along with a few edge case tests
I don't know why but it decided to just cut out a huge chunk of code for some reason
@Geotale Geotale changed the title Paired move truncation Propertly Truncate Paired Single Move Instructions Sep 8, 2024
@Dentomologist
Copy link
Contributor

Can you add a unit test matching the tests you ran on hardware? That would help show what has been tested and prevent any future regressions.

Also, the second commit should be squashed into the first one.

@Geotale
Copy link
Contributor Author

Geotale commented Sep 8, 2024

I can probably commit to the hwtests repo if I can figure out how to get it set up ^^;
I also will definitely squash the commits but I am not confident that I won't have to change things and I'd rather not have to multiple times if possible :D

@Dentomologist
Copy link
Contributor

While it would definitely be good to have that in the hwtests repo, I was primarily thinking about here which gets run on every PR.

@Geotale
Copy link
Contributor Author

Geotale commented Sep 8, 2024

I'm mildly afraid to try that bc this is interpreter-only and would require an entire thing somehow setting that up :,D

@JosJuice
Copy link
Member

JosJuice commented Sep 9, 2024

I would very much like a hwtest. And if we have that, I don't think there's any need to require a unit test on top. Yes, being able to automatically test stuff is nice, but:

  1. This code doesn't get changed often at all, so running the relevant hwtests manually is a surmountable task, and
  2. The time it would take to set up a bunch of unit tests for the interpreter would probably be better spent getting hwtests to run automatically like we run FifoCI, or setting up some other kind of testing system that can run both on console and on Dolphin. Unit tests are inherently less useful than hwtests because we can't run them on hardware.

Geotale and others added 5 commits September 9, 2024 18:56
This passes every test case I could think of which computes each part using bitwise operations rather than any floats
I'll try getting hwtests working soon and if I can't I'll see if anyone else can ^^
@Tilka
Copy link
Member

Tilka commented Sep 14, 2024

I've confirmed with your hwtest (dolphin-emu/hwtests#56) plus isync that the ps_merge*/ps_mr/ps_neg/ps_abs/ps_nabs changes match hardware. If you move the changes for those opcodes (and the shared code) into a separate PR, we can get that merged now and leave ps_sel/ps_rsqrte/ps_sum0/ps_sum1 verification for later.

@Geotale
Copy link
Contributor Author

Geotale commented Sep 14, 2024

Oh right I forgot about sum0 and sum1, ty for reminding me ^^ -- I was planning try to figure out what in the world is failing the 3 specific tests with ps_rsqrte first though :O

@Tilka
Copy link
Member

Tilka commented Sep 15, 2024

Based on some limited testing, I think ps_sel doesn't have the NaN/infinity special case.

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

Successfully merging this pull request may close these issues.

4 participants