[DFG] Compare operations do not respect negative zeros
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Jun 2018 02:28:01 +0000 (02:28 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Jun 2018 02:28:01 +0000 (02:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183729

Reviewed by Saam Barati.

JSTests:

* stress/comparison-ignore-negative-zero.js: Added.
(shouldBe):
(zero):
(negativeZero):
(object.valueOf):
(test):

Source/JavaScriptCore:

Compare operations do not respect negative zeros. So propagating this can
reduce the size of the produced code for negative zero case. This pattern
can be seen in Kraken stanford-crypto-aes.

This also causes an existing bug which converts CompareEq(Int32Only, NonIntAsdouble) to false.
However, NonIntAsdouble includes negative zero, which can be equal to Int32 positive zero.
This issue is covered by fold-based-on-int32-proof-mul-branch.js, and we fix this.

* bytecode/SpeculatedType.cpp:
(JSC::leastUpperBoundOfStrictlyEquivalentSpeculations):
SpecNonIntAsDouble includes negative zero (-0.0), which can be equal to 0 and 0.0.
To emphasize this, we use SpecAnyIntAsDouble | SpecNonIntAsDouble directly instead of
SpecDoubleReal.

* dfg/DFGBackwardsPropagationPhase.cpp:
(JSC::DFG::BackwardsPropagationPhase::propagate):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232567 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/comparison-ignore-negative-zero.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/SpeculatedType.cpp
Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp

index 3842c6c..2179069 100644 (file)
@@ -1,3 +1,17 @@
+2018-06-06  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [DFG] Compare operations do not respect negative zeros
+        https://bugs.webkit.org/show_bug.cgi?id=183729
+
+        Reviewed by Saam Barati.
+
+        * stress/comparison-ignore-negative-zero.js: Added.
+        (shouldBe):
+        (zero):
+        (negativeZero):
+        (object.valueOf):
+        (test):
+
 2018-06-06  Saam Barati  <sbarati@apple.com>
 
         generateConditionsForInstanceOf needs to see if the object has a poly proto structure before assuming it has a constant prototype
diff --git a/JSTests/stress/comparison-ignore-negative-zero.js b/JSTests/stress/comparison-ignore-negative-zero.js
new file mode 100644 (file)
index 0000000..ffd09e3
--- /dev/null
@@ -0,0 +1,65 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + String(actual) + ' ' + String(expected));
+}
+noInline(shouldBe);
+
+function zero()
+{
+    return 0;
+}
+noInline(zero);
+
+function negativeZero()
+{
+    return -0;
+}
+noInline(negativeZero);
+
+var object = {
+    valueOf()
+    {
+        return -0;
+    }
+};
+
+function test()
+{
+    shouldBe(0 < zero(), false);
+    shouldBe(0 < (-zero()), false);
+    shouldBe(0 <= zero(), true);
+    shouldBe(0 <= (-zero()), true);
+    shouldBe(0 > zero(), false);
+    shouldBe(0 > (-zero()), false);
+    shouldBe(0 >= zero(), true);
+    shouldBe(0 >= (-zero()), true);
+    shouldBe(0 == zero(), true);
+    shouldBe(0 == (-zero()), true);
+    shouldBe(0 === zero(), true);
+    shouldBe(0 === (-zero()), true);
+    shouldBe(0 != zero(), false);
+    shouldBe(0 != (-zero()), false);
+    shouldBe(0 !== zero(), false);
+    shouldBe(0 !== (-zero()), false);
+
+    shouldBe(0 < object, false);
+    shouldBe(0 < -object, false);
+    shouldBe(0 <= object, true);
+    shouldBe(0 <= -object, true);
+    shouldBe(0 > object, false);
+    shouldBe(0 > -object, false);
+    shouldBe(0 >= object, true);
+    shouldBe(0 >= -object, true);
+    shouldBe(0 == object, true);
+    shouldBe(0 == -object, true);
+    shouldBe(0 === object, false);
+    shouldBe(0 === -object, true);
+    shouldBe(0 != object, false);
+    shouldBe(0 != -object, false);
+    shouldBe(0 !== object, true);
+    shouldBe(0 !== -object, false);
+}
+noInline(test);
+
+for (var i = 0; i < 1e5; ++i)
+    test();
index 750d278..818dd4b 100644 (file)
@@ -1,3 +1,27 @@
+2018-06-06  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [DFG] Compare operations do not respect negative zeros
+        https://bugs.webkit.org/show_bug.cgi?id=183729
+
+        Reviewed by Saam Barati.
+
+        Compare operations do not respect negative zeros. So propagating this can
+        reduce the size of the produced code for negative zero case. This pattern
+        can be seen in Kraken stanford-crypto-aes.
+
+        This also causes an existing bug which converts CompareEq(Int32Only, NonIntAsdouble) to false.
+        However, NonIntAsdouble includes negative zero, which can be equal to Int32 positive zero.
+        This issue is covered by fold-based-on-int32-proof-mul-branch.js, and we fix this.
+
+        * bytecode/SpeculatedType.cpp:
+        (JSC::leastUpperBoundOfStrictlyEquivalentSpeculations):
+        SpecNonIntAsDouble includes negative zero (-0.0), which can be equal to 0 and 0.0.
+        To emphasize this, we use SpecAnyIntAsDouble | SpecNonIntAsDouble directly instead of
+        SpecDoubleReal.
+
+        * dfg/DFGBackwardsPropagationPhase.cpp:
+        (JSC::DFG::BackwardsPropagationPhase::propagate):
+
 2018-06-06  Saam Barati  <sbarati@apple.com>
 
         generateConditionsForInstanceOf needs to see if the object has a poly proto structure before assuming it has a constant prototype
index 2f14a58..709519d 100644 (file)
@@ -567,8 +567,10 @@ SpeculatedType speculationFromJSType(JSType type)
 
 SpeculatedType leastUpperBoundOfStrictlyEquivalentSpeculations(SpeculatedType type)
 {
-    if (type & (SpecAnyInt | SpecAnyIntAsDouble))
-        type |= (SpecAnyInt | SpecAnyIntAsDouble);
+    // SpecNonIntAsDouble includes negative zero (-0.0), which can be equal to 0 and 0.0 in the context of == and ===.
+    if (type & (SpecAnyInt | SpecAnyIntAsDouble | SpecNonIntAsDouble))
+        type |= (SpecAnyInt | SpecAnyIntAsDouble | SpecNonIntAsDouble);
+
     if (type & SpecString)
         type |= SpecString;
     return type;
index 9547f89..058aacf 100644 (file)
@@ -388,6 +388,19 @@ private:
             break;
         }
 
+        case CompareLess:
+        case CompareLessEq:
+        case CompareGreater:
+        case CompareGreaterEq:
+        case CompareBelow:
+        case CompareBelowEq:
+        case CompareEq:
+        case CompareStrictEq: {
+            node->child1()->mergeFlags(NodeBytecodeUsesAsNumber | NodeBytecodeUsesAsOther);
+            node->child2()->mergeFlags(NodeBytecodeUsesAsNumber | NodeBytecodeUsesAsOther);
+            break;
+        }
+
         case PutByValDirect:
         case PutByVal: {
             m_graph.varArgChild(node, 0)->mergeFlags(NodeBytecodeUsesAsValue);