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

FEXCore/vl64: Fixes int16 encoding #4338

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

Sonicadvance1
Copy link
Member

For some reason when I was writing the tests I got the byte order
incorrect. The type header needs to be in the first byte, not the second
byte.

For some reason when I was writing the tests I got the byte order
incorrect. The type header needs to be in the first byte, not the second
byte.
So when encoding we also test the decode path.
Copy link
Collaborator

@bylaws bylaws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be preferable to drop the bitfield use entirely and just mask and shift as appropriate for all lengths? The current approach is still reasonable though

@Sonicadvance1
Copy link
Member Author

I wonder if it would be preferable to drop the bitfield use entirely and just mask and shift as appropriate for all lengths? The current approach is still reasonable though

The main optimization here is keeping as many values as 8-bit as possible since ~97% fit within the signed 7-bit range that fits within that encoding. The remaining 3% fitting within the 16-bit encoding (14-bit signed value encoded) seems reasonable enough, which is why the 32-bit and 64-bit encoding doesn't try to be any smarter.

@Sonicadvance1 Sonicadvance1 merged commit b3a69af into FEX-Emu:main Feb 8, 2025
12 checks passed
@Sonicadvance1 Sonicadvance1 deleted the fix_vl_int16 branch February 8, 2025 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants