Skip to content

Don't sign extend 16-bit immediates to 32-bit in ppc64le dynrec, fixes #2846 #2850

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

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

classilla
Copy link
Contributor

Description

Previous versions of the ppc32 and ppc64 dynrecs loaded 16-bit immediates without regard for sign extension (the upper 32 bits would be cleared regardless on ppc64). Most things don't care, but apparently the XGA code does, manifesting in broken fonts in Windows 3.11 emulation. This corrects the dynrec to clear the upper bits with a manual load and immediate-OR if it gets a negative uint16_t (positive values can still go through the old fast path, so any performance impact should be minimal).

Makes no changes other than the ppc64le dynrec, no regression risk on other platforms.

Manual testing

Verified functioning on Fedora 38 ppc64le with test pack in #2846 . No regressions noted in SVGA games.

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • [ X] followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@classilla
Copy link
Contributor Author

Hmm. I'm not sure why, but I can't mark this as a "bug."

The same fix should work for ppc32, but I don't have a Linux ppc32 system currently.

@kcgen kcgen added the bug Something isn't working label Sep 9, 2023
@kcgen
Copy link
Member

kcgen commented Sep 9, 2023

That was fast!
Thank you @classilla 🚀

Re labels: I suspect only people with write access to the repo can set labels (currently; will check if we can enable this for all PR contributors).

Ps - if there's areas you're interested in working on, we can add write access so you can create branches straight away in the repo.

@kcgen kcgen self-requested a review September 9, 2023 16:53
@kcgen
Copy link
Member

kcgen commented Sep 9, 2023

I have a 32bit PPC laptop running netBSD. Happy to get it back in working shape if it will help in your PPC efforts. I bought it solely for this type of testing.

@kcgen kcgen merged commit da8e162 into dosbox-staging:main Sep 9, 2023
@johnnovak
Copy link
Member

That was fast! Thank you @classilla 🚀

Yeah, impressive @classilla, plus you're first one filling out the new PR and doing it correctly 🎉 That's a good start 😎

Re labels: I suspect only people with write access to the repo can set labels (currently; will check if we can enable this for all PR contributors).

I suspected this would be the case, so I'm considering bringing back the "type of change" checkboxes perhaphs.

@Grounded0 Grounded0 added CPU/FPU emulation Issues related to CPU or FPU emulation non-x86 Issues related to platforms other than the x86 labels Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CPU/FPU emulation Issues related to CPU or FPU emulation non-x86 Issues related to platforms other than the x86
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants