Using GCC 6.3.1 and targeting ARM64 on Linux, the stress/uint32-to-number-overflows-to-uint52.js JSC stress test (in all variations) fails since r223318 landed. http://trac.webkit.org/browser/webkit/trunk/JSTests/stress/uint32-to-number-overflows-to-uint52.js The compareToLargeNumber() function in that test is the problematic one: > function compareToLargeNumber(value) > { > return (value >>> 0) < 4294967294; > } Here's the generated bytecode output for this function on x86_64, which matches ARM64 before r223318: compareToLargeNumber#D5ozQ8:[0x7ff965a890d0->0x7ff965aa8ba0, NoneFunctionCall, 20 (NeverInline)]: 20 m_instructions; 160 bytes; 2 parameter(s); 8 callee register(s); 6 variable(s); scope at loc3 [ 0] enter [ 1] get_scope loc3 [ 3] mov loc4, loc3 [ 6] check_traps [ 7] urshift loc6, arg1, Int32: 0(const0) [ 11] unsigned loc6, loc6 [ 14] less loc6, loc6, Double: 4751297606871678976, 4294967294.000000(const1) [ 18] ret loc6 Here's the currently generated bytecode on ARM64, missing op_unsigned: compareToLargeNumber#D5ozQ8:[0x7f809890d0->0x7f809a8ba0, NoneFunctionCall, 17 (NeverInline)]: 17 m_instructions; 136 bytes; 2 parameter(s); 8 callee register(s); 6 variable(s); scope at loc3 [ 0] enter [ 1] get_scope loc3 [ 3] mov loc4, loc3 [ 6] check_traps [ 7] urshift loc6, arg1, Int32: 0(const0) [ 11] below loc6, loc6, Double: 4751297606871678976, 4294967294.000000(const1) [ 15] ret loc6 ----- Problem can be tracked to the double-to-int32_t conversion in the helper isUInt32() lambda in BinaryOpNode::emitBytecode(). On x86_64, the 4294967294.0 double value is converted to a negative 32-bit of value -2147483648, which prevents the m_shouldToUnsignedResult value for the op_urshift BinaryOpNode to be set to false, which ends up emitting op_unsigned and thus passing the test. On ARM64, the same double value is converted to 2147483647. The GCC-generated code does this using the plain fcvtzs instruction: > 6fe208: 940039b2 bl 70c8d0 <JSC::NumberNode::value() const [clone .isra.109]> > 6fe20c: 1e780000 fcvtzs w0, d0 Result of this is that both left and right sides of the binary op are recognized as an uint32 value, short-cutting op_unsigned and failing the test.
Identical problem also appears on ARM Thumb2, with the float value being converted to the same integer value as on ARM64. Here's the generated code that performs conversion: > 2352c8: ed98 7b06 vldr d7, [r8, #24] > ... > 2352d2: eefd 6bc7 vcvt.s32.f64 s13, d7go
Created attachment 324106 [details] Patch
Can confirm this fixes issues on all ARM flavors. Thanks! I'll yield the review to JSC devs.
Comment on attachment 324106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324106&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1998 > + if (value.isInt32()) { > + if (value.asInt32() >= 0) Nit: these two if-statements can be folded into one.
Comment on attachment 324106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324106&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1998 >> + if (value.asInt32() >= 0) > > Nit: these two if-statements can be folded into one. Nice, fixed.
Created attachment 324107 [details] Patch
Comment on attachment 324107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324107&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1997 > + auto value = jsNumber(static_cast<NumberNode*>(node)->value()); > + if (value.isInt32() && value.asInt32() >= 0) I thought the goal was to treat 32-bit signed JSValue representation as unsigned? Am I missing something?
Comment on attachment 324107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324107&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1997 >> + if (value.isInt32() && value.asInt32() >= 0) > > I thought the goal was to treat 32-bit signed JSValue representation as unsigned? Am I missing something? No. The goal of this is returning `UInt32Result::Constant` if the constant integer value is within uint32_t range. (this is `op_unsigned(lhs) < rhsConstantUInt32`'s rhsConstantUInt32 detection.) In this patch, we allow [0, INT32_MAX] for this case.
Comment on attachment 324107 [details] Patch Thank you
Comment on attachment 324107 [details] Patch Clearing flags on attachment: 324107 Committed r223745: <https://trac.webkit.org/changeset/223745>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35568730>