Attachment 339182 Details for Bug 185149 – patch for landing

[patch] patch for landing

c-backup.diff (text/plain), 5.76 KB, created by Saam Barati on 2018-04-30 21:39:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-04-30 21:39:40 PDT

Size: 5.76 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 231192)
>+++ JSTests/ChangeLog	(working copy)
>@@ -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
>Index: JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js
>===================================================================
>--- JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js	(nonexistent)
>+++ JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js	(working copy)
>@@ -0,0 +1,7 @@
>+//@ runDefault("--jitPolicyScale=0", "--useConcurrentJIT=false")
>+
>+let bar;
>+for (let i = 0; i < 20; ++i) {
>+  bar = i ** 0;
>+  bar + '';
>+}
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 231192)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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: Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp	(revision 231192)
>+++ Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp	(working copy)
>@@ -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()
>     {

Actions: View | Formatted Diff | Diff

Attachments on bug 185149: 339162 | 339182