Attachment 339162 Details for Bug 185149 – patch

[patch] patch

a-backup.diff (text/plain), 5.18 KB, created by Saam Barati on 2018-04-30 16:51:09 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-04-30 16:51:09 PDT

Size: 5.18 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 231180)
>+++ 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 NOBODY (OOPS!).
>+
>+        * stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js: Added.
>+
> 2018-04-29  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r231137.
>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 231172)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,20 @@
>+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 NOBODY (OOPS!).
>+
>+        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.
>+
>+        * dfg/DFGStrengthReductionPhase.cpp:
>+        (JSC::DFG::StrengthReductionPhase::handleNode):
>+        (JSC::DFG::StrengthReductionPhase::convertToLazyJSValue):
>+
> 2018-04-30  Keith Miller  <keith_miller@apple.com>
> 
>         Move the MayBePrototype JSCell header bit to InlineTypeFlags
>Index: Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp	(revision 231172)
>+++ 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;
>                 }
>             }
>@@ -883,8 +881,7 @@ private:
>                 if (lastIndex < string.length())
>                     builder.append(string, lastIndex, string.length() - lastIndex);
>                 
>-                m_node->convertToLazyJSConstant(
>-                    m_graph, LazyJSValue::newString(m_graph, builder.toString()));
>+                convertToLazyJSValue(m_node, LazyJSValue::newString(m_graph, builder.toString()));
>             }
> 
>             m_node->origin = origin;
>@@ -946,6 +943,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()
>     {

Flags:

fpizlo: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 185149: 339162 | 339182