Should not predict OtherObj for ToThis with primitive types under strict mode
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 31 Jan 2016 23:05:10 +0000 (23:05 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 31 Jan 2016 23:05:10 +0000 (23:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=153544

Reviewed by Filip Pizlo.

Currently, ToThis predicates OtherObj for primitive values.
But it's not true in strict mode.
In strict mode, ToThis does nothing on primitive values.

In this patch, we

1. fix prediction. Handles primitive types in strict mode. And we also handles StringObject.
2. convert it to Identity if the argument should be predicted as primitive types.

This optimization is important to implement Primitive.prototype.methods[1].
Otherwise, we always got BadType OSR exits.

[1]: https://bugs.webkit.org/show_bug.cgi?id=143889

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::fixupToThis):
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* tests/stress/to-this-boolean.js: Added.
(Boolean.prototype.negate):
(Boolean.prototype.negate2):
* tests/stress/to-this-double.js: Added.
(Number.prototype.negate):
* tests/stress/to-this-int32.js: Added.
(Number.prototype.negate):
* tests/stress/to-this-int52.js: Added.
(Number.prototype.negate):
* tests/stress/to-this-number.js: Added.
(Number.prototype.negate):
* tests/stress/to-this-string.js: Added.
(String.prototype.prefix):
(String.prototype.first):
(String.prototype.second):
* tests/stress/to-this-symbol.js: Added.
(Symbol.prototype.identity):
(Symbol.prototype.identity2):

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

12 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/tests/stress/to-this-boolean.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/to-this-double.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/to-this-int32.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/to-this-int52.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/to-this-number.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/to-this-string.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/to-this-symbol.js [new file with mode: 0644]

index 98b3c34..c439021 100644 (file)
@@ -1,3 +1,52 @@
+2016-01-31  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Should not predict OtherObj for ToThis with primitive types under strict mode
+        https://bugs.webkit.org/show_bug.cgi?id=153544
+
+        Reviewed by Filip Pizlo.
+
+        Currently, ToThis predicates OtherObj for primitive values.
+        But it's not true in strict mode.
+        In strict mode, ToThis does nothing on primitive values.
+
+        In this patch, we
+
+        1. fix prediction. Handles primitive types in strict mode. And we also handles StringObject.
+        2. convert it to Identity if the argument should be predicted as primitive types.
+
+        This optimization is important to implement Primitive.prototype.methods[1].
+        Otherwise, we always got BadType OSR exits.
+
+        [1]: https://bugs.webkit.org/show_bug.cgi?id=143889
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::foldConstants):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        (JSC::DFG::FixupPhase::fixupToThis):
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        (JSC::DFG::PredictionPropagationPhase::propagate):
+        * tests/stress/to-this-boolean.js: Added.
+        (Boolean.prototype.negate):
+        (Boolean.prototype.negate2):
+        * tests/stress/to-this-double.js: Added.
+        (Number.prototype.negate):
+        * tests/stress/to-this-int32.js: Added.
+        (Number.prototype.negate):
+        * tests/stress/to-this-int52.js: Added.
+        (Number.prototype.negate):
+        * tests/stress/to-this-number.js: Added.
+        (Number.prototype.negate):
+        * tests/stress/to-this-string.js: Added.
+        (String.prototype.prefix):
+        (String.prototype.first):
+        (String.prototype.second):
+        * tests/stress/to-this-symbol.js: Added.
+        (Symbol.prototype.identity):
+        (Symbol.prototype.identity2):
+
 2016-01-31  Guillaume Emont  <guijemont@igalia.com>
 
         [mips] don't save to a callee saved register too early
index 9eec63c..d7c1bff 100644 (file)
@@ -1691,10 +1691,21 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
     case ToThis: {
         AbstractValue& source = forNode(node->child1());
         AbstractValue& destination = forNode(node);
-            
-        if (m_graph.executableFor(node->origin.semantic)->isStrictMode())
+
+        if (source.m_type == SpecStringObject) {
+            m_state.setFoundConstants(true);
+            destination = source;
+            break;
+        }
+
+        if (m_graph.executableFor(node->origin.semantic)->isStrictMode()) {
+            if (!(source.m_type & ~(SpecFullNumber | SpecBoolean | SpecString | SpecSymbol))) {
+                m_state.setFoundConstants(true);
+                destination = source;
+                break;
+            }
             destination.makeHeapTop();
-        else {
+        else {
             destination = source;
             destination.merge(SpecObject);
         }
index 3a56e35..e5be8ef 100644 (file)
@@ -552,7 +552,7 @@ private:
                 changed = true;
                 break;
             }
-                
+
             case Check: {
                 alreadyHandled = true;
                 m_interpreter.execute(indexInBlock);
index c9d5abf..2f235ad 100644 (file)
@@ -1003,30 +1003,7 @@ private:
         }
             
         case ToThis: {
-            ECMAMode ecmaMode = m_graph.executableFor(node->origin.semantic)->isStrictMode() ? StrictMode : NotStrictMode;
-
-            if (node->child1()->shouldSpeculateOther()) {
-                if (ecmaMode == StrictMode) {
-                    fixEdge<OtherUse>(node->child1());
-                    node->convertToIdentity();
-                    break;
-                }
-
-                m_insertionSet.insertNode(
-                    m_indexInBlock, SpecNone, Check, node->origin,
-                    Edge(node->child1().node(), OtherUse));
-                observeUseKindOnNode<OtherUse>(node->child1().node());
-                m_graph.convertToConstant(
-                    node, m_graph.globalThisObjectFor(node->origin.semantic));
-                break;
-            }
-            
-            if (isFinalObjectSpeculation(node->child1()->prediction())) {
-                fixEdge<FinalObjectUse>(node->child1());
-                node->convertToIdentity();
-                break;
-            }
-            
+            fixupToThis(node);
             break;
         }
             
@@ -1592,6 +1569,85 @@ private:
             node->convertToIdentity();
         }
     }
+
+    void fixupToThis(Node* node)
+    {
+        ECMAMode ecmaMode = m_graph.executableFor(node->origin.semantic)->isStrictMode() ? StrictMode : NotStrictMode;
+
+        if (ecmaMode == StrictMode) {
+            if (node->child1()->shouldSpeculateBoolean()) {
+                fixEdge<BooleanUse>(node->child1());
+                node->convertToIdentity();
+                return;
+            }
+
+            if (node->child1()->shouldSpeculateInt32()) {
+                fixEdge<Int32Use>(node->child1());
+                node->convertToIdentity();
+                return;
+            }
+
+            if (enableInt52() && node->child1()->shouldSpeculateMachineInt()) {
+                fixEdge<Int52RepUse>(node->child1());
+                node->convertToIdentity();
+                node->setResult(NodeResultInt52);
+                return;
+            }
+
+            if (node->child1()->shouldSpeculateNumber()) {
+                fixEdge<DoubleRepUse>(node->child1());
+                node->convertToIdentity();
+                node->setResult(NodeResultDouble);
+                return;
+            }
+
+            if (node->child1()->shouldSpeculateSymbol()) {
+                fixEdge<SymbolUse>(node->child1());
+                node->convertToIdentity();
+                return;
+            }
+
+            if (node->child1()->shouldSpeculateStringIdent()) {
+                fixEdge<StringIdentUse>(node->child1());
+                node->convertToIdentity();
+                return;
+            }
+
+            if (node->child1()->shouldSpeculateString()) {
+                fixEdge<StringUse>(node->child1());
+                node->convertToIdentity();
+                return;
+            }
+        }
+
+        if (node->child1()->shouldSpeculateOther()) {
+            if (ecmaMode == StrictMode) {
+                fixEdge<OtherUse>(node->child1());
+                node->convertToIdentity();
+                return;
+            }
+
+            m_insertionSet.insertNode(
+                m_indexInBlock, SpecNone, Check, node->origin,
+                Edge(node->child1().node(), OtherUse));
+            observeUseKindOnNode<OtherUse>(node->child1().node());
+            m_graph.convertToConstant(
+                node, m_graph.globalThisObjectFor(node->origin.semantic));
+            return;
+        }
+
+        if (node->child1()->shouldSpeculateStringObject()) {
+            fixEdge<StringObjectUse>(node->child1());
+            node->convertToIdentity();
+            return;
+        }
+
+        if (isFinalObjectSpeculation(node->child1()->prediction())) {
+            fixEdge<FinalObjectUse>(node->child1());
+            node->convertToIdentity();
+            return;
+        }
+    }
     
     void fixupToPrimitive(Node* node)
     {
index 57998cf..fa48248 100644 (file)
@@ -489,11 +489,58 @@ private:
         }
 
         case ToThis: {
+            // ToThis in methods for primitive types should speculate primitive types in strict mode.
+            ECMAMode ecmaMode = m_graph.executableFor(node->origin.semantic)->isStrictMode() ? StrictMode : NotStrictMode;
+            if (ecmaMode == StrictMode) {
+                if (node->child1()->shouldSpeculateBoolean()) {
+                    changed |= mergePrediction(SpecBoolean);
+                    break;
+                }
+
+                if (node->child1()->shouldSpeculateInt32()) {
+                    changed |= mergePrediction(SpecInt32);
+                    break;
+                }
+
+                if (enableInt52() && node->child1()->shouldSpeculateMachineInt()) {
+                    changed |= mergePrediction(SpecMachineInt);
+                    break;
+                }
+
+                if (node->child1()->shouldSpeculateNumber()) {
+                    changed |= mergePrediction(SpecMachineInt);
+                    break;
+                }
+
+                if (node->child1()->shouldSpeculateSymbol()) {
+                    changed |= mergePrediction(SpecSymbol);
+                    break;
+                }
+
+                if (node->child1()->shouldSpeculateStringIdent()) {
+                    changed |= mergePrediction(SpecStringIdent);
+                    break;
+                }
+
+                if (node->child1()->shouldSpeculateString()) {
+                    changed |= mergePrediction(SpecString);
+                    break;
+                }
+            } else {
+                if (node->child1()->shouldSpeculateString()) {
+                    changed |= mergePrediction(SpecStringObject);
+                    break;
+                }
+            }
+
             SpeculatedType prediction = node->child1()->prediction();
             if (prediction) {
                 if (prediction & ~SpecObject) {
-                    prediction &= SpecObject;
-                    prediction = mergeSpeculations(prediction, SpecObjectOther);
+                    // Wrapper objects are created only in sloppy mode.
+                    if (ecmaMode != StrictMode) {
+                        prediction &= SpecObject;
+                        prediction = mergeSpeculations(prediction, SpecObjectOther);
+                    }
                 }
                 changed |= mergePrediction(prediction);
             }
diff --git a/Source/JavaScriptCore/tests/stress/to-this-boolean.js b/Source/JavaScriptCore/tests/stress/to-this-boolean.js
new file mode 100644 (file)
index 0000000..e5b3b76
--- /dev/null
@@ -0,0 +1,22 @@
+Boolean.prototype.negate = function ()
+{
+    "use strict";
+    return !this;
+};
+noInline(Boolean.prototype.negate);
+
+for (var i = 0; i < 1e4; ++i)
+    (i % 4 === 0).negate();
+Boolean.prototype.negate.call(true);
+
+for (var i = 0; i < 1e4; ++i)
+    Boolean.prototype.negate.call(i);
+
+Boolean.prototype.negate2 = function ()
+{
+    "use strict";
+    return !this;
+};
+
+for (var i = 0; i < 1e4; ++i)
+    (true).negate2();
diff --git a/Source/JavaScriptCore/tests/stress/to-this-double.js b/Source/JavaScriptCore/tests/stress/to-this-double.js
new file mode 100644 (file)
index 0000000..34b1728
--- /dev/null
@@ -0,0 +1,8 @@
+Number.prototype.negate = function ()
+{
+    "use strict";
+    return -this;
+};
+
+for (var i = 1; i < 1e4; ++i)
+    (4.24242).negate();
diff --git a/Source/JavaScriptCore/tests/stress/to-this-int32.js b/Source/JavaScriptCore/tests/stress/to-this-int32.js
new file mode 100644 (file)
index 0000000..7cf0c2a
--- /dev/null
@@ -0,0 +1,8 @@
+Number.prototype.negate = function ()
+{
+    "use strict";
+    return -this;
+};
+
+for (var i = 1; i < 1e4; ++i)
+    (0x424242).negate();
diff --git a/Source/JavaScriptCore/tests/stress/to-this-int52.js b/Source/JavaScriptCore/tests/stress/to-this-int52.js
new file mode 100644 (file)
index 0000000..145f7a9
--- /dev/null
@@ -0,0 +1,8 @@
+Number.prototype.negate = function ()
+{
+    "use strict";
+    return -this;
+};
+
+for (var i = 1; i < 1e4; ++i)
+    (0xfffffff * 100000).negate();
diff --git a/Source/JavaScriptCore/tests/stress/to-this-number.js b/Source/JavaScriptCore/tests/stress/to-this-number.js
new file mode 100644 (file)
index 0000000..2f13a9f
--- /dev/null
@@ -0,0 +1,20 @@
+Number.prototype.negate = function ()
+{
+    "use strict";
+    return -this;
+};
+noInline(Number.prototype.negate);
+
+for (var i = 0; i < 1e4; ++i)
+    (i % 3 === 0 ? -i : i).negate();
+
+for (var i = 0; i < 1e4; ++i)
+    ((i % 3 === 0 ? -i : i) * 0.2).negate();
+
+for (var i = 0; i < 1e4; ++i)
+    ((i % 3 === 0 ? -i : i) * 1000000).negate();
+
+Number.prototype.negate.call(-20000);
+
+for (var i = 0; i < 1e4; ++i)
+    Number.prototype.negate.call(i % 2 === 0);
diff --git a/Source/JavaScriptCore/tests/stress/to-this-string.js b/Source/JavaScriptCore/tests/stress/to-this-string.js
new file mode 100644 (file)
index 0000000..0d4f9b1
--- /dev/null
@@ -0,0 +1,27 @@
+String.prototype.prefix = function (string)
+{
+    "use strict";
+    // ToThis should be converted to Identity.
+    return string + this;
+};
+noInline(String.prototype.prefix);
+
+String.prototype.first = function (string)
+{
+    return this.second(string);
+};
+
+String.prototype.second = function (string)
+{
+    // Duplicate ToThis(in first) should be converted to Identity.
+    return this + string;
+};
+noInline(String.prototype.first);
+
+
+for (var i = 0; i < 1e4; ++i)
+    String(i).prefix("Hello");
+
+
+for (var i = 0; i < 1e4; ++i)
+    String(i).first("Hello");
diff --git a/Source/JavaScriptCore/tests/stress/to-this-symbol.js b/Source/JavaScriptCore/tests/stress/to-this-symbol.js
new file mode 100644 (file)
index 0000000..e4520ab
--- /dev/null
@@ -0,0 +1,18 @@
+Symbol.prototype.identity = function ()
+{
+    "use strict";
+    return this;
+};
+noInline(Symbol.prototype.identity);
+
+Symbol.prototype.identity2 = function ()
+{
+    "use strict";
+    return this;
+};
+
+for (var i = 1; i < 1e4; ++i)
+    Symbol.prototype.identity.call(Symbol(i));
+
+for (var i = 1; i < 1e4; ++i)
+    Symbol.prototype.identity2.call(Symbol(i));