From: sbarati@apple.com Date: Tue, 1 May 2018 06:04:33 +0000 (+0000) Subject: ToString constant folds without preserving checks, causing us to break assumptions... X-Git-Url: http://git.webkit.org/?p=WebKit-https.git;a=commitdiff_plain;h=eece4cc56d0cff411f2af32960ecffda8705d8a9 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 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 --- diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog index b0c93f3..636a83c 100644 --- a/JSTests/ChangeLog +++ b/JSTests/ChangeLog @@ -1,3 +1,13 @@ +2018-04-30 Saam Barati + + 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 + + + Reviewed by Filip Pizlo. + + * stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js: Added. + 2018-04-29 Filip Pizlo 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 index 0000000..f4c6d40 --- /dev/null +++ b/JSTests/stress/keep-checks-when-converting-to-lazy-js-constant-in-strength-reduction.js @@ -0,0 +1,7 @@ +//@ runDefault("--jitPolicyScale=0", "--useConcurrentJIT=false") + +let bar; +for (let i = 0; i < 20; ++i) { + bar = i ** 0; + bar + ''; +} diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index be14673..478be0a 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,24 @@ +2018-04-30 Saam Barati + + 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 + + + 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 LICM shouldn't hoist nodes if hoisted nodes exited in that code block diff --git a/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp b/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp index 5c1be68..91ee1c3 100644 --- a/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp +++ b/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp @@ -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() {