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

Remove split restore/save from mm7 #4107

Closed
wants to merge 2 commits into from
Closed

Conversation

pmatos
Copy link
Collaborator

@pmatos pmatos commented Oct 8, 2024

No description provided.

@pmatos
Copy link
Collaborator Author

pmatos commented Oct 8, 2024

@Sonicadvance1 I have a feeling that the split in saving/restoring mm7 is something due to past representations of the registers. I can't find a reason for these now. While trying to understand why, I go back a few years to when you split a bunch of code into several files, one of them x87.cpp.

Removing this split improves instcountci and passes all the tests, so it doesn't feel like it's necessary. Can you take a look? If it's indeed necessary, we should probably add a test that fails when the split in save and restore is removed.

@Sonicadvance1
Copy link
Member

@Sonicadvance1 I have a feeling that the split in saving/restoring mm7 is something due to past representations of the registers. I can't find a reason for these now. While trying to understand why, I go back a few years to when you split a bunch of code into several files, one of them x87.cpp.

Removing this split improves instcountci and passes all the tests, so it doesn't feel like it's necessary. Can you take a look? If it's indeed necessary, we should probably add a test that fails when the split in save and restore is removed.

The split is due to the instruction only saving 10-bytes rather than 16-bytes per register. We're taking advantage of overlapping writes to simulate the same behaviour. The final store still needs to be 10-bytes in order to ensure we don't accidentally write past the end of a page, and also ensure we don't overwrite some other data after the the tail.

This change will break that edge case, we just don't have ASM unittests in place for it.

@pmatos pmatos closed this Oct 8, 2024
@pmatos pmatos deleted the SplitMM7 branch October 8, 2024 21:18
Sonicadvance1 added a commit to Sonicadvance1/FEX that referenced this pull request Oct 8, 2024
…equired

The instruction definition only allows these instructions to load/store
94 or 108 bytes, not affecting any bytes afterwards. This is a bit
awkward because 80-bit x87 registers are stored at the end.

FEX has an optimization today where it uses overlapping loads and stores
for the first seven x87 registers, and a split loadstore for the final
register. This ensures that we get the correct data while reducing the
number of loadstores.

We didn't have a unittest in place to ensure we only ever write the
correct amount of data, so changes like in FEX-Emu#4107 which look correct from
an initial glance, would have resulted in broken behaviour.

This unittest ensures both that the instructions don't try to access
beyond the end of the page, and also ensures that they don't overwrite
subsequent data. Making sure that potentially broken behaviour doesn't
make its way in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants