Merge pull request #1201 from TomHarte/IDIVYuck

Improve IDIV marginally; require acceptable failures to have a reason.

InstructionSets/x86/Decoder.cpp

@@ -33,7 +33,7 @@ std::pair<int, typename Decoder<model>::InstructionT> Decoder<model>::decode(con
 
 /// Sets the operation and verifies that the current repetition, if any, is compatible, discarding it otherwise.
 #define SetOperation(op)	\
-	operation_ = rep_operation(op, repetition_);
+	operation_ = rep_operation<model>(op, repetition_);
 
 /// Helper macro for those that follow.
 #define SetOpSrcDestSize(op, src, dest, size)	\

InstructionSets/x86/Implementation/Arithmetic.hpp

@@ -227,7 +227,7 @@ void div(
 	destination_high = dividend % source;
 }
 
-template <typename IntT, typename ContextT>
+template <bool invert, typename IntT, typename ContextT>
 void idiv(
 	modify_t<IntT> destination_high,
 	modify_t<IntT> destination_low,
@@ -279,7 +279,14 @@ void idiv(
 	// TEMPORARY HACK. Will not work with DWords.
 	using sIntT = typename std::make_signed<IntT>::type;
 	const int32_t dividend = (sIntT(destination_high) << (8 * sizeof(IntT))) + destination_low;
-	const auto result = dividend / sIntT(source);
+	auto result = dividend / sIntT(source);
+
+	// An 8086 quirk: rep IDIV performs an IDIV that switches the sign on its result,
+	// due to reuse of an internal flag.
+	if constexpr (invert) {
+		result = -result;
+	}
+
 	if(sIntT(result) != result) {
 		interrupt(Interrupt::DivideError, context);
 		return;

InstructionSets/x86/Implementation/PerformImplementation.hpp

@@ -207,10 +207,11 @@ template <
 			Primitive::test<IntT>(destination_r(), source_r(), context);
 		return;
 
-		case Operation::MUL:	Primitive::mul<IntT>(pair_high(), pair_low(), source_r(), context);			return;
+		case Operation::MUL:		Primitive::mul<IntT>(pair_high(), pair_low(), source_r(), context);			return;
-		case Operation::IMUL_1:	Primitive::imul<IntT>(pair_high(), pair_low(), source_r(), context);		return;
+		case Operation::IMUL_1:		Primitive::imul<IntT>(pair_high(), pair_low(), source_r(), context);		return;
-		case Operation::DIV:	Primitive::div<IntT>(pair_high(), pair_low(), source_r(), context);			return;
+		case Operation::DIV:		Primitive::div<IntT>(pair_high(), pair_low(), source_r(), context);			return;
-		case Operation::IDIV:	Primitive::idiv<IntT>(pair_high(), pair_low(), source_r(), context);		return;
+		case Operation::IDIV:		Primitive::idiv<false, IntT>(pair_high(), pair_low(), source_r(), context);	return;
+		case Operation::IDIV_REP:	Primitive::idiv<true, IntT>(pair_high(), pair_low(), source_r(), context);	return;
 
 		case Operation::INC:	Primitive::inc<IntT>(destination_rmw(), context);		break;
 		case Operation::DEC:	Primitive::dec<IntT>(destination_rmw(), context);		break;

InstructionSets/x86/Instruction.cpp

@@ -105,14 +105,15 @@ std::string InstructionSet::x86::to_string(Operation operation, DataSize size, M
 		case Operation::HLT:	return "hlt";
 		case Operation::WAIT:	return "wait";
 
-		case Operation::ADC:	return "adc";
+		case Operation::ADC:		return "adc";
-		case Operation::ADD:	return "add";
+		case Operation::ADD:		return "add";
-		case Operation::SBB:	return "sbb";
+		case Operation::SBB:		return "sbb";
-		case Operation::SUB:	return "sub";
+		case Operation::SUB:		return "sub";
-		case Operation::MUL:	return "mul";
+		case Operation::MUL:		return "mul";
-		case Operation::IMUL_1:	return "imul";
+		case Operation::IMUL_1:		return "imul";
-		case Operation::DIV:	return "div";
+		case Operation::DIV:		return "div";
-		case Operation::IDIV:	return "idiv";
+		case Operation::IDIV:		return "idiv";
+		case Operation::IDIV_REP:	return "idiv";
 
 		case Operation::INC:	return "inc";
 		case Operation::DEC:	return "dec";

InstructionSets/x86/Instruction.hpp

@@ -231,6 +231,8 @@ enum class Operation: uint8_t {
 	SETMOC,
 	/// Set destination to ~0.
 	SETMO,
+	/// Perform an IDIV and negative the result.
+	IDIV_REP,
 
 	//
 	// 80186 additions.
@@ -468,14 +470,24 @@ enum class Source: uint8_t {
 	/// getter is used).
 	IndirectNoBase = Indirect - 1,
 };
+constexpr bool is_register(Source source) {
+	return source < Source::None;
+}
 
 enum class Repetition: uint8_t {
 	None, RepE, RepNE, Rep,
 };
 
 /// @returns @c true if @c operation supports repetition mode @c repetition; @c false otherwise.
+template <Model model>
 constexpr Operation rep_operation(Operation operation, Repetition repetition) {
 	switch(operation) {
+		case Operation::IDIV:
+			if constexpr (model == Model::i8086) {
+				return repetition != Repetition::None ? Operation::IDIV_REP : Operation::IDIV;
+			}
+		[[fallthrough]];
+
 		default: return operation;
 
 		case Operation::INS:

OSBindings/Mac/Clock SignalTests/8088Tests.mm

@@ -387,16 +387,17 @@ struct FailedExecution {
 
 @implementation i8088Tests {
 	std::vector<FailedExecution> execution_failures;
+	std::vector<FailedExecution> permitted_failures;
 	ExecutionSupport execution_support;
 }
 
 - (NSArray<NSString *> *)testFiles {
 	NSString *path = [NSString stringWithUTF8String:TestSuiteHome];
 	NSSet *allowList = [NSSet setWithArray:@[
-		// Current execution failures:
+		// Current execution failures, albeit all permitted:
-//		@"D4.json.gz",		// AAM
+		@"D4.json.gz",		// AAM
-//		@"F6.7.json.gz",	// IDIV
+		@"F6.7.json.gz",	// IDIV byte
-//		@"F7.7.json.gz",	// IDIV
+		@"F7.7.json.gz",	// IDIV word
 	]];
 
 	NSSet *ignoreList = nil;
@@ -639,53 +640,110 @@ struct FailedExecution {
 	const bool registersEqual = intended_registers == execution_support.registers;
 	const bool flagsEqual = (intended_flags.get() & flags_mask) == (execution_support.flags.get() & flags_mask);
 
-	if(!flagsEqual || !registersEqual || !ramEqual) {
+	// Exit if no issues were found.
-		FailedExecution failure;
+	if(flagsEqual && registersEqual && ramEqual) {
-		failure.instruction = decoded.second;
+		return;
-		failure.test_name = std::string([test[@"name"] UTF8String]);
+	}
 
-		NSMutableArray<NSString *> *reasons = [[NSMutableArray alloc] init];
+	// Presume this is a genuine failure.
-		if(!flagsEqual) {
+	std::vector<FailedExecution> *failure_list = &execution_failures;
-			Flags difference;
+
-			difference.set((intended_flags.get() ^ execution_support.flags.get()) & flags_mask);
+	// Redirect it if it's an acceptable failure.
-			[reasons addObject:
+	using Operation = InstructionSet::x86::Operation;
-				[NSString stringWithFormat:@"flags differs; errors in %s",
+
-					difference.to_string().c_str()]];
+	// AAM 00h throws its exception only after modifying flags in an undocumented manner;
+	// I'm not too concerned about this because AAM 00h is an undocumented usage of 00h,
+	// not even supported by NEC amongst others, and the proper exception is being thrown.
+	if(decoded.second.operation() == Operation::AAM && !decoded.second.operand()) {
+		failure_list = &permitted_failures;
+	}
+
+	// IDIV_REP: for reasons I don't understand, sometimes the test set doesn't increment
+	// the IP across a REP_IDIV. I don't think (?) this correlates to real 8086 behaviour.
+	// More research required, but for now I'm not treating this as a roadblock.
+	if(decoded.second.operation() == Operation::IDIV_REP) {
+		Registers advanced_registers = intended_registers;
+		advanced_registers.ip_ += decoded.first;
+		if(advanced_registers == execution_support.registers && ramEqual && flagsEqual) {
+			failure_list = &permitted_failures;
 		}
-		if(!registersEqual) {
+	}
-			NSMutableArray<NSString *> *registers = [[NSMutableArray alloc] init];
+
+	// IDIV[_REP] byte: the test cases sometimes throw even when I can't see why they should,
+	// and other x86 emulations also don't throw. I guess — guess! — an 8086-specific oddity
+	// deviates from the x86 average here. So I'm also permitting these for now.
+	if(
+		decoded.second.operation_size() == InstructionSet::x86::DataSize::Byte &&
+		(decoded.second.operation() == Operation::IDIV_REP || decoded.second.operation() == Operation::IDIV)
+	) {
+		if(intended_registers.sp() == execution_support.registers.sp() - 6) {
+			Registers non_exception_registers = intended_registers;
+			non_exception_registers.ip() = execution_support.registers.ip();
+			non_exception_registers.sp() = execution_support.registers.sp();
+			non_exception_registers.ax() = execution_support.registers.ax();
+			non_exception_registers.cs() = execution_support.registers.cs();
+
+			if(non_exception_registers == execution_support.registers) {
+				failure_list = &permitted_failures;
+			}
+		}
+	}
+
+	// LEA from a register is undefined behaviour and throws on processors beyond the 8086.
+	if(decoded.second.operation() == Operation::LEA && InstructionSet::x86::is_register(decoded.second.source().source())) {
+		failure_list = &permitted_failures;
+	}
+
+	if(failure_list == &execution_failures) {
+		printf("Fail: %d\n", int(decoded.second.operation()));
+	}
+
+	// Record a failure.
+	FailedExecution failure;
+	failure.instruction = decoded.second;
+	failure.test_name = std::string([test[@"name"] UTF8String]);
+
+	NSMutableArray<NSString *> *reasons = [[NSMutableArray alloc] init];
+	if(!flagsEqual) {
+		Flags difference;
+		difference.set((intended_flags.get() ^ execution_support.flags.get()) & flags_mask);
+		[reasons addObject:
+			[NSString stringWithFormat:@"flags differs; errors in %s",
+				difference.to_string().c_str()]];
+	}
+	if(!registersEqual) {
+		NSMutableArray<NSString *> *registers = [[NSMutableArray alloc] init];
 #define Reg(x)	\
 	if(intended_registers.x() != execution_support.registers.x())	\
 		[registers addObject:	\
 			[NSString stringWithFormat:	\
 				@#x" is %04x rather than %04x", execution_support.registers.x(), intended_registers.x()]];
 
-			Reg(ax);
+		Reg(ax);
-			Reg(cx);
+		Reg(cx);
-			Reg(dx);
+		Reg(dx);
-			Reg(bx);
+		Reg(bx);
-			Reg(sp);
+		Reg(sp);
-			Reg(bp);
+		Reg(bp);
-			Reg(si);
+		Reg(si);
-			Reg(di);
+		Reg(di);
-			Reg(ip);
+		Reg(ip);
-			Reg(es);
+		Reg(es);
-			Reg(cs);
+		Reg(cs);
-			Reg(ds);
+		Reg(ds);
-			Reg(ss);
+		Reg(ss);
 
 #undef Reg
-			[reasons addObject:[NSString stringWithFormat:
+		[reasons addObject:[NSString stringWithFormat:
-				@"registers don't match: %@", [registers componentsJoinedByString:@", "]
+			@"registers don't match: %@", [registers componentsJoinedByString:@", "]
-			]];
+		]];
-		}
-		if(!ramEqual) {
-			[reasons addObject:@"RAM contents don't match"];
-		}
-
-		failure.reason = std::string([reasons componentsJoinedByString:@"; "].UTF8String);
-		execution_failures.push_back(std::move(failure));
 	}
+	if(!ramEqual) {
+		[reasons addObject:@"RAM contents don't match"];
+	}
+
+	failure.reason = std::string([reasons componentsJoinedByString:@"; "].UTF8String);
+	failure_list->push_back(std::move(failure));
 }
 
 - (void)printFailures:(NSArray<NSString *> *)failures {
@@ -744,20 +802,16 @@ struct FailedExecution {
 	}
 
 	// Lock in current failure rate.
-	XCTAssertLessThanOrEqual(execution_failures.size(), 4009);
+	XCTAssertLessThanOrEqual(execution_failures.size(), 0);
-
-	// Current accepted failures:
-	//	* 2484 instances of LEA from a register, which officially has undefined results;
-	//	* 42 instances of AAM 00h for which I can't figure out what to do with flags; and
-	//	* 1486 instances of IDIV, most either with a rep or repne that on the 8086 specifically negatives the result,
-	//		but some admittedly still unexplained (primarily setting overflow even though the result doesn't overflow;
-	//		a couple of online 8086 emulators also didn't throw so maybe this is an 8086 quirk?)
 
 	for(const auto &failure: execution_failures) {
 		NSLog(@"Failed %s — %s", failure.test_name.c_str(), failure.reason.c_str());
 	}
+	for(const auto &failure: permitted_failures) {
+		NSLog(@"Permitted failure of %s — %s", failure.test_name.c_str(), failure.reason.c_str());
+	}
 
-	NSLog(@"Files with failures were: %@", failures);
+	NSLog(@"Files with failures, permitted or otherwise, were: %@", failures);
 }
 
 @end