From 90e50cc2ccd0bf858403d635cf0c264de2d60ea6 Mon Sep 17 00:00:00 2001 From: Sigurd Schneider Date: Tue, 23 Jan 2018 19:47:41 +0100 Subject: [PATCH] [turbofan] Add effects to StringAt operators Add effect input and output to String.p.char[Code]At/codePointAt. This is necessary to fix an hard to reproduce bug, a repro for which is included. However, the only way to get the repro included in this CL to fail is to run it with the patch of 873382: [turbofan] Speculate on bounds checks for String#char[Code]At but WITHOUT this patch. This fixes a scheduling problem triggered by 873382 that caused a bounds check to get scheduled after the associated access. Bug: v8:7326 Change-Id: I4b97c1726caac92ff8f74c23df2788f0ecfb1304 Reviewed-on: https://chromium-review.googlesource.com/881781 Reviewed-by: Jaroslav Sevcik Commit-Queue: Sigurd Schneider Cr-Commit-Position: refs/heads/master@{#50832} --- src/compiler/js-builtin-reducer.cc | 84 ++----------------- src/compiler/js-call-reducer.cc | 7 +- .../js-native-context-specialization.cc | 13 ++- src/compiler/simplified-operator.cc | 28 +++++-- .../regress/regress-stringAt-boundsCheck.js | 18 ++++ 5 files changed, 62 insertions(+), 88 deletions(-) create mode 100644 test/mjsunit/regress/regress-stringAt-boundsCheck.js diff --git a/src/compiler/js-builtin-reducer.cc b/src/compiler/js-builtin-reducer.cc index 7ff2bf6d5e..33fb496734 100644 --- a/src/compiler/js-builtin-reducer.cc +++ b/src/compiler/js-builtin-reducer.cc @@ -2032,81 +2032,10 @@ Reduction JSBuiltinReducer::ReduceStringIteratorNext(Node* node) { Node* vtrue0; { done_true = jsgraph()->FalseConstant(); - Node* lead = graph()->NewNode(simplified()->StringCharCodeAt(), string, - index, if_true0); - - // branch1: if ((lead & 0xFC00) === 0xD800) - Node* check1 = - graph()->NewNode(simplified()->NumberEqual(), - graph()->NewNode(simplified()->NumberBitwiseAnd(), - lead, jsgraph()->Constant(0xFC00)), - jsgraph()->Constant(0xD800)); - Node* branch1 = graph()->NewNode(common()->Branch(BranchHint::kFalse), - check1, if_true0); - Node* if_true1 = graph()->NewNode(common()->IfTrue(), branch1); - Node* vtrue1; - { - Node* next_index = graph()->NewNode(simplified()->NumberAdd(), index, - jsgraph()->OneConstant()); - // branch2: if ((index + 1) < length) - Node* check2 = graph()->NewNode(simplified()->NumberLessThan(), - next_index, length); - Node* branch2 = graph()->NewNode(common()->Branch(BranchHint::kTrue), - check2, if_true1); - Node* if_true2 = graph()->NewNode(common()->IfTrue(), branch2); - Node* vtrue2; - { - Node* trail = graph()->NewNode(simplified()->StringCharCodeAt(), - string, next_index, if_true2); - // branch3: if ((trail & 0xFC00) === 0xDC00) - Node* check3 = graph()->NewNode( - simplified()->NumberEqual(), - graph()->NewNode(simplified()->NumberBitwiseAnd(), trail, - jsgraph()->Constant(0xFC00)), - jsgraph()->Constant(0xDC00)); - Node* branch3 = graph()->NewNode(common()->Branch(BranchHint::kTrue), - check3, if_true2); - Node* if_true3 = graph()->NewNode(common()->IfTrue(), branch3); - Node* vtrue3; - { - vtrue3 = graph()->NewNode( - simplified()->NumberBitwiseOr(), -// Need to swap the order for big-endian platforms -#if V8_TARGET_BIG_ENDIAN - graph()->NewNode(simplified()->NumberShiftLeft(), lead, - jsgraph()->Constant(16)), - trail); -#else - graph()->NewNode(simplified()->NumberShiftLeft(), trail, - jsgraph()->Constant(16)), - lead); -#endif - } - - Node* if_false3 = graph()->NewNode(common()->IfFalse(), branch3); - Node* vfalse3 = lead; - if_true2 = graph()->NewNode(common()->Merge(2), if_true3, if_false3); - vtrue2 = - graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2), - vtrue3, vfalse3, if_true2); - } - - Node* if_false2 = graph()->NewNode(common()->IfFalse(), branch2); - Node* vfalse2 = lead; - if_true1 = graph()->NewNode(common()->Merge(2), if_true2, if_false2); - vtrue1 = - graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2), - vtrue2, vfalse2, if_true1); - } - - Node* if_false1 = graph()->NewNode(common()->IfFalse(), branch1); - Node* vfalse1 = lead; - if_true0 = graph()->NewNode(common()->Merge(2), if_true1, if_false1); - vtrue0 = - graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2), - vtrue1, vfalse1, if_true0); + Node* codepoint = etrue0 = graph()->NewNode( + simplified()->StringCodePointAt(), string, index, etrue0, if_true0); vtrue0 = graph()->NewNode( - simplified()->StringFromCodePoint(UnicodeEncoding::UTF16), vtrue0); + simplified()->StringFromCodePoint(UnicodeEncoding::UTF16), codepoint); // Update iterator.[[NextIndex]] Node* char_length = @@ -2173,6 +2102,7 @@ Reduction JSBuiltinReducer::ReduceStringSlice(Node* node) { Node* if_false = graph()->NewNode(common()->IfFalse(), branch); Node* vfalse; + Node* efalse; { // We need to convince TurboFan that {receiver_length}-1 is a valid // Unsigned32 value, so we just apply NumberToUint32 to the result @@ -2181,14 +2111,16 @@ Reduction JSBuiltinReducer::ReduceStringSlice(Node* node) { graph()->NewNode(simplified()->NumberSubtract(), receiver_length, jsgraph()->OneConstant()); index = graph()->NewNode(simplified()->NumberToUint32(), index); - vfalse = graph()->NewNode(simplified()->StringCharAt(), receiver, index, - if_false); + vfalse = efalse = graph()->NewNode(simplified()->StringCharAt(), + receiver, index, effect, if_false); } control = graph()->NewNode(common()->Merge(2), if_true, if_false); Node* value = graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2), vtrue, vfalse, control); + effect = + graph()->NewNode(common()->EffectPhi(2), effect, efalse, control); ReplaceWithValue(node, value, effect, control); return Replace(value); } diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index bf2e37323d..ae35e86318 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -3900,9 +3900,9 @@ Reduction JSCallReducer::ReduceStringPrototypeStringAt( index = graph()->NewNode(simplified()->MaskIndexWithBound(), index, receiver_length); - - Node* vtrue = - graph()->NewNode(string_access_operator, receiver, index, if_true); + Node* etrue; + Node* vtrue = etrue = graph()->NewNode(string_access_operator, receiver, + index, effect, if_true); // Return the {default_return} otherwise. Node* if_false = graph()->NewNode(common()->IfFalse(), branch); @@ -3911,6 +3911,7 @@ Reduction JSCallReducer::ReduceStringPrototypeStringAt( control = graph()->NewNode(common()->Merge(2), if_true, if_false); Node* value = graph()->NewNode( common()->Phi(MachineRepresentation::kTagged, 2), vtrue, vfalse, control); + effect = graph()->NewNode(common()->EffectPhi(2), etrue, effect, control); ReplaceWithValue(node, value, effect, control); return Replace(value); diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index b2f8c567e2..b60b2b8fe2 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -2524,13 +2524,16 @@ Node* JSNativeContextSpecialization::BuildIndexedStringLoad( graph()->NewNode(simplified()->MaskIndexWithBound(), index, length); Node* if_true = graph()->NewNode(common()->IfTrue(), branch); - Node* vtrue = graph()->NewNode(simplified()->StringCharAt(), receiver, - masked_index, if_true); + Node* etrue; + Node* vtrue = etrue = graph()->NewNode( + simplified()->StringCharAt(), receiver, masked_index, *effect, if_true); Node* if_false = graph()->NewNode(common()->IfFalse(), branch); Node* vfalse = jsgraph()->UndefinedConstant(); *control = graph()->NewNode(common()->Merge(2), if_true, if_false); + *effect = + graph()->NewNode(common()->EffectPhi(2), etrue, *effect, *control); return graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2), vtrue, vfalse, *control); } else { @@ -2543,8 +2546,10 @@ Node* JSNativeContextSpecialization::BuildIndexedStringLoad( graph()->NewNode(simplified()->MaskIndexWithBound(), index, length); // Return the character from the {receiver} as single character string. - return graph()->NewNode(simplified()->StringCharAt(), receiver, - masked_index, *control); + Node* value = *effect = + graph()->NewNode(simplified()->StringCharAt(), receiver, masked_index, + *effect, *control); + return value; } } diff --git a/src/compiler/simplified-operator.cc b/src/compiler/simplified-operator.cc index 9978bae122..487a854213 100644 --- a/src/compiler/simplified-operator.cc +++ b/src/compiler/simplified-operator.cc @@ -655,11 +655,6 @@ bool operator==(CheckMinusZeroParameters const& lhs, V(NumberToUint8Clamped, Operator::kNoProperties, 1, 0) \ V(NumberSilenceNaN, Operator::kNoProperties, 1, 0) \ V(StringToNumber, Operator::kNoProperties, 1, 0) \ - V(StringCharAt, Operator::kNoProperties, 2, 1) \ - V(StringCharCodeAt, Operator::kNoProperties, 2, 1) \ - V(SeqStringCharCodeAt, Operator::kNoProperties, 2, 1) \ - V(StringCodePointAt, Operator::kNoProperties, 2, 1) \ - V(SeqStringCodePointAt, Operator::kNoProperties, 2, 1) \ V(StringFromCharCode, Operator::kNoProperties, 1, 0) \ V(StringIndexOf, Operator::kNoProperties, 3, 0) \ V(StringLength, Operator::kNoProperties, 1, 0) \ @@ -710,6 +705,13 @@ bool operator==(CheckMinusZeroParameters const& lhs, V(NewConsString, Operator::kNoProperties, 3, 0) \ V(MaskIndexWithBound, Operator::kNoProperties, 2, 0) +#define EFFECT_DEPENDENT_OP_LIST(V) \ + V(StringCharAt, Operator::kNoProperties, 2, 1) \ + V(StringCharCodeAt, Operator::kNoProperties, 2, 1) \ + V(SeqStringCharCodeAt, Operator::kNoProperties, 2, 1) \ + V(StringCodePointAt, Operator::kNoProperties, 2, 1) \ + V(SeqStringCodePointAt, Operator::kNoProperties, 2, 1) + #define SPECULATIVE_NUMBER_BINOP_LIST(V) \ SIMPLIFIED_SPECULATIVE_NUMBER_BINOP_LIST(V) \ V(SpeculativeNumberEqual) \ @@ -755,6 +757,20 @@ struct SimplifiedOperatorGlobalCache final { PURE_OP_LIST(PURE) #undef PURE +#define EFFECT_DEPENDENT(Name, properties, value_input_count, \ + control_input_count) \ + struct Name##Operator final : public Operator { \ + Name##Operator() \ + : Operator(IrOpcode::k##Name, \ + Operator::kNoDeopt | Operator::kNoWrite | \ + Operator::kNoThrow | properties, \ + #Name, value_input_count, 1, control_input_count, 1, 1, \ + 0) {} \ + }; \ + Name##Operator k##Name; + EFFECT_DEPENDENT_OP_LIST(EFFECT_DEPENDENT) +#undef EFFECT_DEPENDENT + #define CHECKED(Name, value_input_count, value_output_count) \ struct Name##Operator final : public Operator { \ Name##Operator() \ @@ -1032,6 +1048,7 @@ SimplifiedOperatorBuilder::SimplifiedOperatorBuilder(Zone* zone) #define GET_FROM_CACHE(Name, ...) \ const Operator* SimplifiedOperatorBuilder::Name() { return &cache_.k##Name; } PURE_OP_LIST(GET_FROM_CACHE) +EFFECT_DEPENDENT_OP_LIST(GET_FROM_CACHE) CHECKED_OP_LIST(GET_FROM_CACHE) GET_FROM_CACHE(ArrayBufferWasNeutered) GET_FROM_CACHE(ArgumentsFrame) @@ -1463,6 +1480,7 @@ const Operator* SimplifiedOperatorBuilder::TransitionAndStoreNonNumberElement( } #undef PURE_OP_LIST +#undef EFFECT_DEPENDENT_OP_LIST #undef SPECULATIVE_NUMBER_BINOP_LIST #undef CHECKED_WITH_FEEDBACK_OP_LIST #undef CHECKED_OP_LIST diff --git a/test/mjsunit/regress/regress-stringAt-boundsCheck.js b/test/mjsunit/regress/regress-stringAt-boundsCheck.js new file mode 100644 index 0000000000..2e14aa027c --- /dev/null +++ b/test/mjsunit/regress/regress-stringAt-boundsCheck.js @@ -0,0 +1,18 @@ +// Copyright 2018 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --opt --allow-natives-syntax + +(() => { + function f(u) { + for (var j = 0; j < 20; ++j) { + print("" + u.codePointAt()); + } + } + + f("test"); + f("foo"); + %OptimizeFunctionOnNextCall(f); + f(""); +})();