-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
New x87 Unit Tests & Opcodes #184
Conversation
- FSTP Refactored and added Unit Tests - FSTSW Unit Tests - FSTCW Unit Tests - FCOMPP Fixes and Unit Tests
- FCOM added and added Unit Tests - FCOMP consolidated to call FCOM then Pop, added Unit Tests - FISTP added Unit Tests - FLD1 added Unit Tests - FST Refactored and added Unit Tests - FSTP Consolidated to call FST then Pop - FSQURT added edge-cases for FPU Exceptions and added Unit Tests - Added `ClearExceptions()` to the FPU Register to clear Exception Flags from both the Status Word and Control Word
…into x87-unit-tests
var actualFPUCodes = mbbsEmuCpuRegisters.Fpu.StatusWord & FPU_CODE_MASK; | ||
|
||
Assert.Equal(expectedFlags, actualFPUCodes); | ||
Assert.Equal(7, mbbsEmuCpuRegisters.Fpu.GetStackTop()); |
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.
popping 0 -> 7? Is that valid or one of those stack underflow exceptions which sets C1?
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.
It'd technically be a stack underflow. What'd happen on the next push is it pushes the value (ST0 == [0])
I don't set C1 or worry about those flags ATM because I haven't run across a module that verifies those values... yet 😉
|
||
namespace MBBSEmu.Tests.CPU | ||
{ | ||
public class FISTP_Tests : CpuTestBase |
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.
useful to add tests with different rounding modes as specified by the RC field in the FPU control word?
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.
Right now it's defaulting to rounding away from Zero. We can change it to respect the RC field eventually
mbbsEmuCpuCore.FpuStack[1] = ST0Value; | ||
|
||
var instructions = new Assembler(16); | ||
instructions.fstp(st1); |
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.
is there an assert that ST1 == ST0Value?
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.
In this test, ST1 becomes ST0 after the FSTP instruction because of the FPU POP. So we set the FPU Stack Top to [1], FSTP saves the result to ST1 [0] and the pop sets ST0 to [0].
MBBSEmu/CPU/CPUCore.cs
Outdated
private void Op_Fsubrp() | ||
{ | ||
var valueToSubtractFrom = GetOperandValueDouble(_currentInstruction.Op0Kind, EnumOperandType.Destination); | ||
var valueToSubtract = GetOperandValueDouble(_currentInstruction.Op1Kind, EnumOperandType.Source); |
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.
FSUBR => (DST = SRC - DST)
At least with those EnumOperandTypes, it looks like DST - SRC. I don't really have all the context here, so just double check this is right
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.
Yeah -- this was backwards and a little confusing. I fixed the Unit Test to properly reflect subtracting destination (STi/1) from source (ST0). This is how Iced handles the disassembly of FSUBR. Where ST0 has to be the SOURCE operand.
Fixes #171