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

Fix RCL Implementation #596

Merged
merged 1 commit into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions MBBSEmu.Tests/CPU/RLC_Tests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using Iced.Intel;
using Xunit;
using static Iced.Intel.AssemblerRegisters;

namespace MBBSEmu.Tests.CPU
{
public class RCL_Tests : CpuTestBase
{
[Theory]
[InlineData(0x8000, 1, false, 0x0000, true)] // Rotate left with carry in, resulting in CF set
[InlineData(0x8000, 1, true, 0x0001, true)] // Rotate left with carry in, resulting in CF set, and LSB set from previous CF
[InlineData(0x0001, 1, false, 0x0002, false)] // Simple rotate left
[InlineData(0x0000, 1, true, 0x0001, false)] // Rotate with carry flag set, no bit set in value
[InlineData(0xFFFF, 4, false, 0xFFF7, true)] // Rotate left multiple times
public void Op_Rcl_16_Test(ushort axValue, byte bitsToRotate, bool initialCarryFlag, ushort expectedResult,
bool expectedCarryFlag)
{
Reset();
mbbsEmuCpuRegisters.AX = axValue;
mbbsEmuCpuRegisters.CarryFlag = initialCarryFlag;

var instructions = new Assembler(16);
instructions.rcl(ax, bitsToRotate);
CreateCodeSegment(instructions);

mbbsEmuCpuCore.Tick();

Assert.Equal(expectedResult, mbbsEmuCpuRegisters.AX);
Assert.Equal(expectedCarryFlag, mbbsEmuCpuRegisters.CarryFlag);
}

[Theory]
[InlineData(0x80, 1, false, 0x00, true)] // Rotate left with carry in, resulting in CF set
[InlineData(0x80, 1, true, 0x01, true)] // Rotate left with carry in, resulting in CF set, and LSB set from previous CF
[InlineData(0x01, 1, false, 0x02, false)] // Simple rotate left
[InlineData(0x00, 1, true, 0x01, false)] // Rotate with carry flag set, no bit set in value
[InlineData(0xFF, 4, false, 0xF7, true)] // Rotate left multiple times
public void Op_Rcl_8_Test(byte alValue, byte bitsToRotate, bool initialCarryFlag, ushort expectedResult,
bool expectedCarryFlag)
{
Reset();
mbbsEmuCpuRegisters.AL = alValue;
mbbsEmuCpuRegisters.CarryFlag = initialCarryFlag;

var instructions = new Assembler(16);
instructions.rcl(al, bitsToRotate);
CreateCodeSegment(instructions);

mbbsEmuCpuCore.Tick();

Assert.Equal(expectedResult, mbbsEmuCpuRegisters.AL);
Assert.Equal(expectedCarryFlag, mbbsEmuCpuRegisters.CarryFlag);

}
}
}
26 changes: 15 additions & 11 deletions MBBSEmu/CPU/CPUCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ public void Tick()
};

//Set this value to TRUE if you want the CPU to break after each instruction
var CPUDebugBreak = true;
var CPUDebugBreak = false;

//Evaluate Breakpoints
if (CPUBreakpoints.Contains(_currentInstructionPointer))
Expand Down Expand Up @@ -1641,6 +1641,10 @@ private ushort Op_Rcr_16()
}
}

/// <summary>
/// Performs a left circular rotate (RCL) operation on the current operation size.
/// </summary>
/// <exception cref="Exception"></exception>
[MethodImpl(OpcodeCompilerOptimizations)]
private void Op_Rcl()
{
Expand All @@ -1665,17 +1669,17 @@ private byte Op_Rcl_8()
var result = destination;
for (var i = 0; i < source; i++)
{
//Determine the CF Value after rotation+carry
// Determine the CF Value after rotation+carry
var newCFValue = result.IsBitSet(7);

//Perform Rotation
// Perform Rotation
result = (byte)(result << 1);

//If CF was set, rotate that value in
// If CF was set, rotate that value in to the LSB (0th bit)
if (Registers.CarryFlag)
result.SetFlag(1);
result |= 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, technically .SetFlag is an extension method we have on byte types, which since they're static, can't modify the underlying instance value. This was one of the bugs with the underlying logic here, because if using the helper method it should be result = result.SetFlag(1), which I felt was overhead for just a |= 1 operation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, subtle


//Set new CF Value
// Set new CF Value
Registers.CarryFlag = newCFValue;
}

Expand All @@ -1694,17 +1698,17 @@ private ushort Op_Rcl_16()
var result = destination;
for (var i = 0; i < source; i++)
{
//Determine the CF Value after rotation+carry
// Determine the CF Value after rotation+carry
var newCFValue = result.IsBitSet(15);

//Perform Rotation
// Perform Rotation
result = (ushort)(result << 1);

//If CF was set, rotate that value in
// If CF was set, rotate that value in to the LSB (0th bit)
if (Registers.CarryFlag)
result.SetFlag(1);
result |= 1;

//Set new CF Value
// Set new CF Value
Registers.CarryFlag = newCFValue;
}

Expand Down
2 changes: 0 additions & 2 deletions MBBSEmu/CPU/ICpuRegisters.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
using Iced.Intel;
using MBBSEmu.Extensions;
using MBBSEmu.Memory;
using System;
using System.Runtime.InteropServices;

namespace MBBSEmu.CPU
{
Expand Down
Loading