-
Notifications
You must be signed in to change notification settings - Fork 197
Implement x87 invalid operation bit on F64 mode #4729
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you copilot |
|
So there are a few things to note here: As is you flush the IOC bit after every operation, it would make sense to only do this at block exit points, or whenever a non-x87 IOC setting instruction is used in a block. I think if you extend the IR json to track IOC setting operations, as the x87 pass already handles every x87 operation directly, you can just check op->SetsIOC in the non-x87 inst case to know when to flush. |
|
|
||
| // Set x87 invalid operation bit if IOC is set | ||
| // Load as 32-bit to avoid size constraint issues | ||
| Ref CurrentIE32 = _LoadContext(OpSize::i32Bit, GPRClass, offsetof(FEXCore::Core::CPUState, flags) + FEXCore::X86State::X87FLAG_IE_LOC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8-bit has a constraint issue on the load side but not the store side? That seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load8/store8 doesn't work. Lets do load32/store32 which sounds more sensible. Thanks.
bf59bd4 to
9c29587
Compare
Ah, yes, I do understand the issue: both bits are sticky, but the arm bit can be set by many other instructions that are not being emulated from x87 so the possibility of reading a wrong value exists. Need to think of a way to turn that into a unit test though. Need to have a think on how to best handle this case. |
e721b2a to
ae80805
Compare
ae80805 to
55aada1
Compare
|
hummm, VIXL doesn't support FPSR, so we might need to skip some tests on the simulator. |
Been trying to create a test where emulating a non-x87 invalid op causes, through an indirect setting of the FPSR flag on the arm side, the x87 IE flag to show up as set. I think this is what you were alluding to with "after any non-x87 float operation, which could set the bit on the arm side but would not on the x86 side." but I can't create a test that does this and nothing comes to mind. Any suggestions? |
OK - found a test that shows the issue mentioned earlier where cumulative FPSR contaminates x87 status word IE flag. 62b625f#diff-2380a45bab6932717d4fe2ae0561fc1a1450f30d8430392c7b3ff4b29958fd5e |
1410a27 to
47fb217
Compare
|
Any comments on the current version of the patch? I am slightly worried I missed some instructions that SetsIOC or haven't set it in some other cases, but after reviewing it, I think I got most cases covered. |
|
|
||
| for (auto [CodeNode, IROp] : IR->GetCode(BlockNode)) { | ||
| // Clear FPSR IOC bit before non-x87 operations that can set it | ||
| if (FEXCore::IR::SetsIOC(IROp->Op) && !FEXCore::IR::LoweredX87(IROp->Op)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cost of doing this for every instruction that isn't an x87 instruction(and can set IOC) is too much and needs to be re-thought out to limit the performance impact.
47fb217 to
108a199
Compare
It attempts to use the IOC ARM64 bit to avoid lengthy checks on operation arguments. This is the F64 counterpart to 726656d .
108a199 to
f4dc7f3
Compare
|
I have a few upcoming changes to x87 stack pass, so I will need to refactor this. moving it to draft. |
It attempts to use the IOC ARM64 bit to avoid lengthy checks on operation arguments.
This is the F64 counterpart to 726656d .