ToString constant folds without preserving checks, causing us to break assumptions...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 May 2018 06:04:33 +0000 (06:04 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 May 2018 06:04:33 +0000 (06:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185149
<rdar://problem/39455917>

Reviewed by Filip Pizlo.

JSTests:

* stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js: Added.

Source/JavaScriptCore:

The bug was that we were deleting checks that we shouldn't have deleted.
This patch makes a helper inside strength reduction that converts to
a LazyJSConstant while maintaining checks, and switches users of the
node API inside strength reduction to instead call the helper function.

This patch also fixes a potential bug where StringReplace and
StringReplaceRegExp may not preserve all their checks.

* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::handleNode):
(JSC::DFG::StrengthReductionPhase::convertToLazyJSValue):

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

JSTests/ChangeLog
JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp

index b0c93f3..636a83c 100644 (file)
@@ -1,3 +1,13 @@
+2018-04-30  Saam Barati  <sbarati@apple.com>
+
+        ToString constant folds without preserving checks, causing us to break assumptions that the code would OSR exit
+        https://bugs.webkit.org/show_bug.cgi?id=185149
+        <rdar://problem/39455917>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js: Added.
+
 2018-04-29  Filip Pizlo  <fpizlo@apple.com>
 
         LICM shouldn't hoist nodes if hoisted nodes exited in that code block
diff --git a/JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js b/JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js
new file mode 100644 (file)
index 0000000..f4c6d40
--- /dev/null
@@ -0,0 +1,7 @@
+//@ runDefault("--jitPolicyScale=0", "--useConcurrentJIT=false")
+
+let bar;
+for (let i = 0; i < 20; ++i) {
+  bar = i ** 0;
+  bar + '';
+}
index be14673..478be0a 100644 (file)
@@ -1,3 +1,24 @@
+2018-04-30  Saam Barati  <sbarati@apple.com>
+
+        ToString constant folds without preserving checks, causing us to break assumptions that the code would OSR exit
+        https://bugs.webkit.org/show_bug.cgi?id=185149
+        <rdar://problem/39455917>
+
+        Reviewed by Filip Pizlo.
+
+        The bug was that we were deleting checks that we shouldn't have deleted.
+        This patch makes a helper inside strength reduction that converts to
+        a LazyJSConstant while maintaining checks, and switches users of the
+        node API inside strength reduction to instead call the helper function.
+        
+        This patch also fixes a potential bug where StringReplace and
+        StringReplaceRegExp may not preserve all their checks.
+
+
+        * dfg/DFGStrengthReductionPhase.cpp:
+        (JSC::DFG::StrengthReductionPhase::handleNode):
+        (JSC::DFG::StrengthReductionPhase::convertToLazyJSValue):
+
 2018-04-29  Filip Pizlo  <fpizlo@apple.com>
 
         LICM shouldn't hoist nodes if hoisted nodes exited in that code block
index 5c1be68..91ee1c3 100644 (file)
@@ -361,8 +361,7 @@ private:
                 StringBuilder builder;
                 builder.append(leftString);
                 builder.append(rightString);
-                m_node->convertToLazyJSConstant(
-                    m_graph, LazyJSValue::newString(m_graph, builder.toString()));
+                convertToLazyJSValue(m_node, LazyJSValue::newString(m_graph, builder.toString()));
                 m_changed = true;
             }
             break;
@@ -389,8 +388,7 @@ private:
             if (!!extraString)
                 builder.append(extraString);
 
-            m_node->convertToLazyJSConstant(
-                m_graph, LazyJSValue::newString(m_graph, builder.toString()));
+            convertToLazyJSValue(m_node, LazyJSValue::newString(m_graph, builder.toString()));
             m_changed = true;
             break;
         }
@@ -412,7 +410,7 @@ private:
                             result = String::numberToStringECMAScript(value.asNumber());
 
                         if (!result.isNull()) {
-                            m_node->convertToLazyJSConstant(m_graph, LazyJSValue::newString(m_graph, result));
+                            convertToLazyJSValue(m_node, LazyJSValue::newString(m_graph, result));
                             m_changed = true;
                         }
                     }
@@ -432,7 +430,7 @@ private:
                 JSValue value = child1->constant()->value();
                 if (value && value.isNumber()) {
                     String result = toStringWithRadix(value.asNumber(), m_node->validRadixConstant());
-                    m_node->convertToLazyJSConstant(m_graph, LazyJSValue::newString(m_graph, result));
+                    convertToLazyJSValue(m_node, LazyJSValue::newString(m_graph, result));
                     m_changed = true;
                 }
             }
@@ -866,6 +864,10 @@ private:
 
             NodeOrigin origin = m_node->origin;
 
+            // Preserve any checks we have.
+            m_insertionSet.insertNode(
+                m_nodeIndex, SpecNone, Check, origin, m_node->children.justChecks());
+
             if (regExp->global()) {
                 m_insertionSet.insertNode(
                     m_nodeIndex, SpecNone, SetRegExpObjectLastIndex, origin,
@@ -883,8 +885,7 @@ private:
                 if (lastIndex < string.length())
                     builder.append(string, lastIndex, string.length() - lastIndex);
                 
-                m_node->convertToLazyJSConstant(
-                    m_graph, LazyJSValue::newString(m_graph, builder.toString()));
+                m_node->convertToLazyJSConstant(m_graph, LazyJSValue::newString(m_graph, builder.toString()));
             }
 
             m_node->origin = origin;
@@ -946,6 +947,12 @@ private:
     {
         convertToIdentityOverChild(1);
     }
+
+    void convertToLazyJSValue(Node* node, LazyJSValue value)
+    {
+        m_insertionSet.insertCheck(m_graph, m_nodeIndex, node);
+        node->convertToLazyJSConstant(m_graph, value);
+    }
     
     void handleCommutativity()
     {