From e69e9eddc7b96083577fc097360bce2555ac5a56 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 22 Apr 2014 19:40:45 -0700 Subject: [PATCH] Change float comparison operators to return false for NaN (bug 6107, r=ds). --HG-- extra : rebase_source : a11c56fb23d6617545def3591ec6100dd143eb3e --- core/logic/smn_float.cpp | 45 ++++++++++++++ core/sm_stringutil.cpp | 5 ++ plugins/include/float.inc | 71 ++++++++--------------- plugins/testsuite/floats.sp | 81 ++++++++++++++++++++++++++ public/jit/x86/assembler-x86.h | 22 +++++-- sourcepawn/jit/BaseRuntime.cpp | 68 +++++++++++----------- sourcepawn/jit/opcodes.h | 17 ++++-- sourcepawn/jit/x86/jit_x86.cpp | 103 ++++++++++++++++++++++++++++++++- sourcepawn/jit/x86/jit_x86.h | 1 + tools/checkout-deps.sh | 1 + 10 files changed, 324 insertions(+), 90 deletions(-) create mode 100644 plugins/testsuite/floats.sp diff --git a/core/logic/smn_float.cpp b/core/logic/smn_float.cpp index 6f18bfc6..9edcaa48 100644 --- a/core/logic/smn_float.cpp +++ b/core/logic/smn_float.cpp @@ -86,6 +86,44 @@ static cell_t sm_FloatDiv(IPluginContext *pCtx, const cell_t *params) return sp_ftoc(val); } +static cell_t sm_FloatGt(IPluginContext *pCtx, const cell_t *params) +{ + return !!(sp_ctof(params[1]) > sp_ctof(params[2])); +} + +static cell_t sm_FloatGe(IPluginContext *pCtx, const cell_t *params) +{ + return !!(sp_ctof(params[1]) >= sp_ctof(params[2])); +} + +static cell_t sm_FloatLt(IPluginContext *pCtx, const cell_t *params) +{ + return !!(sp_ctof(params[1]) < sp_ctof(params[2])); +} + +static cell_t sm_FloatLe(IPluginContext *pCtx, const cell_t *params) +{ + return !!(sp_ctof(params[1]) <= sp_ctof(params[2])); +} + +static cell_t sm_FloatEq(IPluginContext *pCtx, const cell_t *params) +{ + return !!(sp_ctof(params[1]) == sp_ctof(params[2])); +} + +static cell_t sm_FloatNe(IPluginContext *pCtx, const cell_t *params) +{ + return !!(sp_ctof(params[1]) != sp_ctof(params[2])); +} + +static cell_t sm_FloatNot(IPluginContext *pCtx, const cell_t *params) +{ + float val = sp_ctof(params[1]); + if (isnan(val)) + return 1; + return val ? 0 : 1; +} + static cell_t sm_FloatCompare(IPluginContext *pCtx, const cell_t *params) { float val1 = sp_ctof(params[1]); @@ -361,6 +399,13 @@ REGISTER_NATIVES(floatnatives) {"RoundToCeil", sm_RoundToCeil}, {"RoundToFloor", sm_RoundToFloor}, {"RoundToNearest", sm_RoundToNearest}, + {"__FLOAT_GT__", sm_FloatGt}, + {"__FLOAT_GE__", sm_FloatGe}, + {"__FLOAT_LT__", sm_FloatGt}, + {"__FLOAT_LE__", sm_FloatLe}, + {"__FLOAT_EQ__", sm_FloatEq}, + {"__FLOAT_NE__", sm_FloatNe}, + {"__FLOAT_NOT__", sm_FloatNot}, {"FloatCompare", sm_FloatCompare}, {"SquareRoot", sm_SquareRoot}, {"Pow", sm_Pow}, diff --git a/core/sm_stringutil.cpp b/core/sm_stringutil.cpp index 28057ca4..0f4cb258 100644 --- a/core/sm_stringutil.cpp +++ b/core/sm_stringutil.cpp @@ -213,6 +213,11 @@ void AddFloat(char **buf_p, size_t &maxlen, double fval, int width, int prec, in int significant_digits = 0; // number of significant digits written const int MAX_SIGNIFICANT_DIGITS = 16; + if (isnan(fval)) { + AddString(buf_p, maxlen, "NaN", width, prec); + return; + } + // default precision if (prec < 0) { diff --git a/plugins/include/float.inc b/plugins/include/float.inc index 0b060d31..ffa43f20 100644 --- a/plugins/include/float.inc +++ b/plugins/include/float.inc @@ -246,10 +246,25 @@ stock RoundFloat(Float:value) */ #pragma rational Float +native bool:__FLOAT_GT__(Float:a, Float:b); +native bool:__FLOAT_GE__(Float:a, Float:b); +native bool:__FLOAT_LT__(Float:a, Float:b); +native bool:__FLOAT_LE__(Float:a, Float:b); +native bool:__FLOAT_EQ__(Float:a, Float:b); +native bool:__FLOAT_NE__(Float:a, Float:b); +native bool:__FLOAT_NOT__(Float:a); + native Float:operator*(Float:oper1, Float:oper2) = FloatMul; native Float:operator/(Float:oper1, Float:oper2) = FloatDiv; native Float:operator+(Float:oper1, Float:oper2) = FloatAdd; native Float:operator-(Float:oper1, Float:oper2) = FloatSub; +native bool:operator!(Float:oper1) = __FLOAT_NOT__; +native bool:operator>(Float:oper1, Float:oper2) = __FLOAT_GT__; +native bool:operator>=(Float:oper1, Float:oper2) = __FLOAT_GE__; +native bool:operator<(Float:oper1, Float:oper2) = __FLOAT_LT__; +native bool:operator<=(Float:oper1, Float:oper2) = __FLOAT_LE__; +native bool:operator!=(Float:oper1, Float:oper2) = __FLOAT_NE__; +native bool:operator==(Float:oper1, Float:oper2) = __FLOAT_EQ__; stock Float:operator++(Float:oper) { @@ -296,90 +311,54 @@ stock Float:operator-(oper1, Float:oper2) return FloatSub(float(oper1), oper2); } -stock bool:operator==(Float:oper1, Float:oper2) -{ - return FloatCompare(oper1, oper2) == 0; -} - stock bool:operator==(Float:oper1, oper2) { - return FloatCompare(oper1, float(oper2)) == 0; /* "==" is commutative */ -} - -stock bool:operator!=(Float:oper1, Float:oper2) -{ - return FloatCompare(oper1, oper2) != 0; + return __FLOAT_EQ__(oper1, float(oper2)); } stock bool:operator!=(Float:oper1, oper2) { - return FloatCompare(oper1, float(oper2)) != 0; /* "==" is commutative */ -} - -stock bool:operator>(Float:oper1, Float:oper2) -{ - return FloatCompare(oper1, oper2) > 0; + return __FLOAT_NE__(oper1, float(oper2)); } stock bool:operator>(Float:oper1, oper2) { - return FloatCompare(oper1, float(oper2)) > 0; + return __FLOAT_GT__(oper1, float(oper2)); } stock bool:operator>(oper1, Float:oper2) { - return FloatCompare(float(oper1), oper2) > 0; -} - -stock bool:operator>=(Float:oper1, Float:oper2) -{ - return FloatCompare(oper1, oper2) >= 0; + return __FLOAT_GT__(float(oper1), oper2); } stock bool:operator>=(Float:oper1, oper2) { - return FloatCompare(oper1, float(oper2)) >= 0; + return __FLOAT_GE__(oper1, float(oper2)); } stock bool:operator>=(oper1, Float:oper2) { - return FloatCompare(float(oper1), oper2) >= 0; -} - -stock bool:operator<(Float:oper1, Float:oper2) -{ - return FloatCompare(oper1, oper2) < 0; + return __FLOAT_GE__(float(oper1), oper2); } stock bool:operator<(Float:oper1, oper2) { - return FloatCompare(oper1, float(oper2)) < 0; + return __FLOAT_LT__(oper1, float(oper2)); } stock bool:operator<(oper1, Float:oper2) { - return FloatCompare(float(oper1), oper2) < 0; -} - -stock bool:operator<=(Float:oper1, Float:oper2) -{ - return FloatCompare(oper1, oper2) <= 0; + return __FLOAT_LT__(float(oper1), oper2); } stock bool:operator<=(Float:oper1, oper2) { - return FloatCompare(oper1, float(oper2)) <= 0; + return __FLOAT_LE__(oper1, float(oper2)); } stock bool:operator<=(oper1, Float:oper2) { - return FloatCompare(float(oper1), oper2) <= 0; -} - -stock bool:operator!(Float:oper) -{ - return (_:oper & ((-1)/2)) == 0; /* -1 = all bits to 1; /2 = remove most significant bit (sign) - works on both 32bit and 64bit systems; no constant required */ + return __FLOAT_LE__(float(oper1), oper2); } /** diff --git a/plugins/testsuite/floats.sp b/plugins/testsuite/floats.sp new file mode 100644 index 00000000..4592029c --- /dev/null +++ b/plugins/testsuite/floats.sp @@ -0,0 +1,81 @@ +public OnPluginStart() +{ + RegServerCmd("test_floats", TestFloats) +} + +check(bool:got, bool:expect, const String:message[]="") +{ + if (got != expect) { + ThrowError("Check failed", message) + } +} + +public Action:TestFloats(args) +{ + new Float:x = 5.3 + new Float:y = 10.2 + + check(x < y, true) + check(x <= y, true) + check(x > y, false) + check(x >= y, false) + check(x == y, false) + check(x != y, true) + + x = 10.5 + y = 2.3 + + check(x < y, false) + check(x <= y, false) + check(x > y, true) + check(x >= y, true) + check(x == y, false) + check(x != y, true) + + x = 10.5 + y = x + + check(x < y, false) + check(x <= y, true) + check(x > y, false) + check(x >= y, true) + check(x == y, true) + check(x != y, false) + + x = 0.0 + y = 0.0 + new Float:nan = x / y + + check(x < nan, false) + check(x <= nan, false) + check(x > nan, false) + check(x >= nan, false) + check(x == nan, false) + check(x != nan, true) + check(nan < y, false) + check(nan <= y, false) + check(nan > y, false) + check(nan >= y, false) + check(nan == y, false) + check(nan != y, true) + check(nan == nan, false) + check(nan != nan, true) + + x = 10.5 + y = 0.0 + check(!x, false) + check(!y, true) + check(!nan, true) + + y = -2.7 + check(-x == -10.5, true) + check(-y == 2.7, true) + + new String:buffer[32] + Format(buffer, sizeof(buffer), "%f", nan) + check(StrEqual(buffer, "NaN"), true) + + PrintToServer("Tests finished.") + + return Plugin_Stop +} diff --git a/public/jit/x86/assembler-x86.h b/public/jit/x86/assembler-x86.h index 2fa1a380..b766bb54 100644 --- a/public/jit/x86/assembler-x86.h +++ b/public/jit/x86/assembler-x86.h @@ -178,7 +178,11 @@ enum ConditionCode { zero = equal, not_zero = not_equal, less_equal = not_greater, - greater_equal = not_less + below_equal = not_above, + greater_equal = not_less, + above_equal = not_below, + parity = even_parity, + not_parity = odd_parity }; enum Scale { @@ -625,6 +629,9 @@ class AssemblerX86 : public Assembler void fsubr32(const Operand &src) { emit1(0xd8, 5, src); } + void fldz() { + emit2(0xd9, 0xee); + } // Compare st0 with stN. void fucomip(FpuRegister other) { @@ -802,11 +809,16 @@ class AssemblerX86 : public Assembler assert(Features().sse); emit3(0xf3, 0x0f, 0x5e, dest.code, src); } - void ucomiss(FloatRegister left, Register right) { - emit2(0x0f, 0x2e, left.code, right.code); + void xorps(FloatRegister dest, FloatRegister src) { + assert(Features().sse); + emit2(0x0f, 0x57, src.code, dest.code); } - void ucomiss(FloatRegister left, const Operand &right) { - emit2(0x0f, 0x2e, left.code, right); + + void ucomiss(FloatRegister left, FloatRegister right) { + emit2(0x0f, 0x2e, right.code, left.code); + } + void ucomiss(const Operand &left, FloatRegister right) { + emit2(0x0f, 0x2e, right.code, left); } // SSE2-only instructions. diff --git a/sourcepawn/jit/BaseRuntime.cpp b/sourcepawn/jit/BaseRuntime.cpp index 463fc146..0b58c675 100644 --- a/sourcepawn/jit/BaseRuntime.cpp +++ b/sourcepawn/jit/BaseRuntime.cpp @@ -75,45 +75,47 @@ BaseRuntime::~BaseRuntime() free(m_plugin.name); } +struct NativeMapping { + const char *name; + unsigned opcode; +}; + +static const NativeMapping sNativeMap[] = { + { "FloatAbs", OP_FABS }, + { "FloatAdd", OP_FLOATADD }, + { "FloatSub", OP_FLOATSUB }, + { "FloatMul", OP_FLOATMUL }, + { "FloatDiv", OP_FLOATDIV }, + { "float", OP_FLOAT }, + { "FloatCompare", OP_FLOATCMP }, + { "RoundToCeil", OP_RND_TO_CEIL }, + { "RoundToZero", OP_RND_TO_ZERO }, + { "RoundToFloor", OP_RND_TO_FLOOR }, + { "RoundToNearest", OP_RND_TO_NEAREST }, + { "__FLOAT_GT__", OP_FLOAT_GT }, + { "__FLOAT_GE__", OP_FLOAT_GE }, + { "__FLOAT_LT__", OP_FLOAT_LT }, + { "__FLOAT_LE__", OP_FLOAT_LE }, + { "__FLOAT_EQ__", OP_FLOAT_EQ }, + { "__FLOAT_NE__", OP_FLOAT_NE }, + { "__FLOAT_NOT__", OP_FLOAT_NOT }, + { NULL, 0 }, +}; + void BaseRuntime::SetupFloatNativeRemapping() { float_table_ = new floattbl_t[m_plugin.num_natives]; for (size_t i = 0; i < m_plugin.num_natives; i++) { const char *name = m_plugin.natives[i].name; - if (!strcmp(name, "FloatAbs")) { - float_table_[i].found = true; - float_table_[i].index = OP_FABS; - } else if (!strcmp(name, "FloatAdd")) { - float_table_[i].found = true; - float_table_[i].index = OP_FLOATADD; - } else if (!strcmp(name, "FloatSub")) { - float_table_[i].found = true; - float_table_[i].index = OP_FLOATSUB; - } else if (!strcmp(name, "FloatMul")) { - float_table_[i].found = true; - float_table_[i].index = OP_FLOATMUL; - } else if (!strcmp(name, "FloatDiv")) { - float_table_[i].found = true; - float_table_[i].index = OP_FLOATDIV; - } else if (!strcmp(name, "float")) { - float_table_[i].found = true; - float_table_[i].index = OP_FLOAT; - } else if (!strcmp(name, "FloatCompare")) { - float_table_[i].found = true; - float_table_[i].index = OP_FLOATCMP; - } else if (!strcmp(name, "RoundToZero")) { - float_table_[i].found = true; - float_table_[i].index = OP_RND_TO_ZERO; - } else if (!strcmp(name, "RoundToCeil")) { - float_table_[i].found = true; - float_table_[i].index = OP_RND_TO_CEIL; - } else if (!strcmp(name, "RoundToFloor")) { - float_table_[i].found = true; - float_table_[i].index = OP_RND_TO_FLOOR; - } else if (!strcmp(name, "RoundToNearest")) { - float_table_[i].found = true; - float_table_[i].index = OP_RND_TO_NEAREST; + const NativeMapping *iter = sNativeMap; + while (iter->name) { + if (strcmp(name, iter->name) == 0) { + float_table_[i].found = true; + float_table_[i].index = iter->opcode; + break; + } + iter++; } } } diff --git a/sourcepawn/jit/opcodes.h b/sourcepawn/jit/opcodes.h index 048e1492..599b0353 100644 --- a/sourcepawn/jit/opcodes.h +++ b/sourcepawn/jit/opcodes.h @@ -238,15 +238,22 @@ _(ENDPROC, "endproc") \ _(FABS, "fabs") \ _(FLOAT, "float") \ - _(FLOATADD, "floatadd") \ - _(FLOATSUB, "floatsub") \ - _(FLOATMUL, "floatmul") \ - _(FLOATDIV, "floatdiv") \ + _(FLOATADD, "float.add") \ + _(FLOATSUB, "float.sub") \ + _(FLOATMUL, "float.mul") \ + _(FLOATDIV, "float.div") \ _(RND_TO_NEAREST, "round") \ _(RND_TO_FLOOR, "floor") \ _(RND_TO_CEIL, "ceil") \ _(RND_TO_ZERO, "rndtozero") \ - _(FLOATCMP, "floatcmp") + _(FLOATCMP, "float.cmp") \ + _(FLOAT_GT, "float.gt") \ + _(FLOAT_GE, "float.ge") \ + _(FLOAT_LT, "float.lt") \ + _(FLOAT_LE, "float.le") \ + _(FLOAT_NE, "float.ne") \ + _(FLOAT_EQ, "float.eq") \ + _(FLOAT_NOT, "float.not") enum OPCODE { #define _(op, text) OP_##op, diff --git a/sourcepawn/jit/x86/jit_x86.cpp b/sourcepawn/jit/x86/jit_x86.cpp index 74cb7ed7..7eff8c4e 100644 --- a/sourcepawn/jit/x86/jit_x86.cpp +++ b/sourcepawn/jit/x86/jit_x86.cpp @@ -1085,12 +1085,16 @@ Compiler::emitOp(OPCODE op) __ addl(stk, 4); break; + // This is the old float cmp, which returns ordered results. In newly + // compiled code it should not be used or generated. + // + // Note that the checks here are inverted: the test is |rhs OP lhs|. case OP_FLOATCMP: { Label bl, ab, done; if (MacroAssemblerX86::Features().sse) { __ movss(xmm0, Operand(stk, 4)); - __ ucomiss(xmm0, Operand(stk, 0)); + __ ucomiss(Operand(stk, 0), xmm0); } else { __ fld32(Operand(stk, 0)); __ fld32(Operand(stk, 4)); @@ -1111,6 +1115,53 @@ Compiler::emitOp(OPCODE op) break; } + case OP_FLOAT_GT: + emitFloatCmp(above); + break; + + case OP_FLOAT_GE: + emitFloatCmp(above_equal); + break; + + case OP_FLOAT_LE: + emitFloatCmp(below_equal); + break; + + case OP_FLOAT_LT: + emitFloatCmp(below); + break; + + case OP_FLOAT_EQ: + emitFloatCmp(equal); + break; + + case OP_FLOAT_NE: + emitFloatCmp(not_equal); + break; + + case OP_FLOAT_NOT: + { + if (MacroAssemblerX86::Features().sse) { + __ xorps(xmm0, xmm0); + __ ucomiss(Operand(stk, 0), xmm0); + } else { + __ fld32(Operand(stk, 0)); + __ fldz(); + __ fucomip(st1); + __ fstp(st0); + } + + // See emitFloatCmp() - this is a shorter version. + Label done; + __ movl(eax, 1); + __ j(parity, &done); + __ set(zero, r8_al); + __ bind(&done); + + __ addl(stk, 4); + break; + } + case OP_STACK: { cell_t amount = readCell(); @@ -1671,6 +1722,56 @@ Compiler::emitErrorPath(Label *dest, int code) } } +void +Compiler::emitFloatCmp(ConditionCode cc) +{ + unsigned lhs = 4; + unsigned rhs = 0; + if (cc == below || cc == below_equal) { + // NaN results in ZF=1 PF=1 CF=1 + // + // ja/jae check for ZF,CF=0 and CF=0. If we make all relational compares + // look like ja/jae, we'll guarantee all NaN comparisons will fail (which + // would not be true for jb/jbe, unless we checked with jp). + if (cc == below) + cc = above; + else + cc = above_equal; + rhs = 4; + lhs = 0; + } + + if (MacroAssemblerX86::Features().sse) { + __ movss(xmm0, Operand(stk, rhs)); + __ ucomiss(Operand(stk, lhs), xmm0); + } else { + __ fld32(Operand(stk, rhs)); + __ fld32(Operand(stk, lhs)); + __ fucomip(st1); + __ fstp(st0); + } + + // An equal or not-equal needs special handling for the parity bit. + if (cc == equal || cc == not_equal) { + // If NaN, PF=1, ZF=1, and E/Z tests ZF=1. + // + // If NaN, PF=1, ZF=1 and NE/NZ tests Z=0. But, we want any != with NaNs + // to return true, including NaN != NaN. + // + // To make checks simpler, we set |eax| to the expected value of a NaN + // beforehand. This also clears the top bits of |eax| for setcc. + Label done; + __ movl(eax, (cc == equal) ? 0 : 1); + __ j(parity, &done); + __ set(cc, r8_al); + __ bind(&done); + } else { + __ movl(eax, 0); + __ set(cc, r8_al); + } + __ addl(stk, 8); +} + void Compiler::emitErrorPaths() { diff --git a/sourcepawn/jit/x86/jit_x86.h b/sourcepawn/jit/x86/jit_x86.h index 5fa7dc7e..daaec0f2 100644 --- a/sourcepawn/jit/x86/jit_x86.h +++ b/sourcepawn/jit/x86/jit_x86.h @@ -105,6 +105,7 @@ class Compiler void emitCheckAddress(Register reg); void emitErrorPath(Label *dest, int code); void emitErrorPaths(); + void emitFloatCmp(ConditionCode cc); ExternalAddress cipAddr() { sp_context_t *ctx = rt_->GetBaseContext()->GetCtx(); diff --git a/tools/checkout-deps.sh b/tools/checkout-deps.sh index 7daaa8d4..ff05a8d1 100755 --- a/tools/checkout-deps.sh +++ b/tools/checkout-deps.sh @@ -57,6 +57,7 @@ checkout () hg clone https://hg.alliedmods.net/$path/$name else cd $name + sed -i 's/http:/https:/g' .hg/hgrc hg pull -u cd .. fi