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

Jit_Integer: Use SHLX, SHRX, SARX #9385

Merged
merged 2 commits into from Jan 15, 2021
Merged

Conversation

merryhime
Copy link
Contributor

Avoid unnecessary MOV instructions and RCX use when BMI2 is available.

@merryhime
Copy link
Contributor Author

merryhime commented Dec 29, 2020

Implementing this exposed a bug where upper 32 bits of GPR registers were not zero (from existing JIT code it seems intentional for these bits to always be zero).

This bug exists in the existing implementation but is less commonly hit in the existing code because in order for it to be triggered it required a == s for a srwx instruction as well as a previous instruction sequence resulting in non-zero bits in the upper half of a GPR.

There are two ways to fix this:
(a) Never assume the upper half of a GPR is all zeros.
(b) Fix all code that violates the assumption that upper half of a GPR is all zeros.

After some debugging, the above only occurs in cases when Rs is in memory.

@merryhime merryhime marked this pull request as ready for review December 29, 2020 23:52
@merryhime
Copy link
Contributor Author

Fixed.

In srwx, allowing Rs to reference memory had resulted in a 64-bit load rather than the desired 32-bit load, resulting in invalid data in the upper half of the register which was then shifted to the right.

<0.5% speed improvement in povray.

Suggested by @Sintendo

Co-authored-by: Sintendo <bram.speeckaert@gmail.com>
@merryhime
Copy link
Contributor Author

@Sintendo Thanks, makes sense to me, applied.

@lioncash lioncash merged commit aba179e into dolphin-emu:master Jan 15, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants