From 91ed540ee662e15a9a33a420848f2e171f2999d1 Mon Sep 17 00:00:00 2001 From: bmeurer Date: Wed, 7 Sep 2016 21:20:09 -0700 Subject: [PATCH] [turbofan] Revert "Avoid overflow checks on SpeculativeNumberAdd/Subtract/Multiply." The optimization is not correct for unsigned output types, and we the overall complexity seems too high. We need to find a better way to take into account the input/output type restrictions. Also added a regression test for the unsigned output bug. BUG=v8:5267,v8:5270,v8:5357 TBR=jarin@chromium.org Review-Url: https://codereview.chromium.org/2320013002 Cr-Commit-Position: refs/heads/master@{#39262} --- src/compiler/representation-change.cc | 2 - src/compiler/representation-change.h | 4 - src/compiler/simplified-lowering.cc | 147 ++++++++++++-------------- test/mjsunit/regress/regress-5357.js | 17 +++ 4 files changed, 86 insertions(+), 84 deletions(-) create mode 100644 test/mjsunit/regress/regress-5357.js diff --git a/src/compiler/representation-change.cc b/src/compiler/representation-change.cc index 6da77053e5..e4a2f42b6c 100644 --- a/src/compiler/representation-change.cc +++ b/src/compiler/representation-change.cc @@ -751,10 +751,8 @@ const Operator* RepresentationChanger::Int32OverflowOperatorFor( const Operator* RepresentationChanger::Uint32OperatorFor( IrOpcode::Value opcode) { switch (opcode) { - case IrOpcode::kSpeculativeNumberAdd: case IrOpcode::kNumberAdd: return machine()->Int32Add(); - case IrOpcode::kSpeculativeNumberSubtract: case IrOpcode::kNumberSubtract: return machine()->Int32Sub(); case IrOpcode::kSpeculativeNumberMultiply: diff --git a/src/compiler/representation-change.h b/src/compiler/representation-change.h index ebe948eeee..fbb4cb5c95 100644 --- a/src/compiler/representation-change.h +++ b/src/compiler/representation-change.h @@ -35,10 +35,6 @@ class Truncation final { bool IsUsedAsFloat64() const { return LessGeneral(kind_, TruncationKind::kFloat64); } - bool IdentifiesMinusZeroAndZero() { - return LessGeneral(kind_, TruncationKind::kWord32) || - LessGeneral(kind_, TruncationKind::kBool); - } bool IdentifiesNaNAndZero() { return LessGeneral(kind_, TruncationKind::kWord32) || LessGeneral(kind_, TruncationKind::kBool); diff --git a/src/compiler/simplified-lowering.cc b/src/compiler/simplified-lowering.cc index 8a7513d101..f50ba3ccfc 100644 --- a/src/compiler/simplified-lowering.cc +++ b/src/compiler/simplified-lowering.cc @@ -241,12 +241,6 @@ class RepresentationSelector { bool weakened() const { return weakened_; } void set_restriction_type(Type* type) { restriction_type_ = type; } Type* restriction_type() const { return restriction_type_; } - void set_unrestricted_feedback_type(Type* type) { - unrestricted_feedback_type_ = type; - } - Type* unrestricted_feedback_type() const { - return unrestricted_feedback_type_; - } private: enum State : uint8_t { kUnvisited, kPushed, kVisited, kQueued }; @@ -257,7 +251,6 @@ class RepresentationSelector { Type* restriction_type_ = Type::Any(); Type* feedback_type_ = nullptr; - Type* unrestricted_feedback_type_ = nullptr; bool weakened_ = false; }; @@ -371,11 +364,6 @@ class RepresentationSelector { return type == nullptr ? Type::None() : type; } - Type* UnrestrictedFeedbackTypeOf(Node* node) { - Type* type = GetInfo(node)->unrestricted_feedback_type(); - return type == nullptr ? NodeProperties::GetType(node) : type; - } - Type* TypePhi(Node* node) { int arity = node->op()->ValueInputCount(); Type* type = FeedbackTypeOf(node->InputAt(0)); @@ -418,11 +406,13 @@ class RepresentationSelector { SIMPLIFIED_NUMBER_BINOP_LIST(DECLARE_CASE) #undef DECLARE_CASE -#define DECLARE_CASE(Name) \ - case IrOpcode::k##Name: { \ - new_type = op_typer_.Name(FeedbackTypeOf(node->InputAt(0)), \ - FeedbackTypeOf(node->InputAt(1))); \ - break; \ +#define DECLARE_CASE(Name) \ + case IrOpcode::k##Name: { \ + new_type = \ + Type::Intersect(op_typer_.Name(FeedbackTypeOf(node->InputAt(0)), \ + FeedbackTypeOf(node->InputAt(1))), \ + info->restriction_type(), graph_zone()); \ + break; \ } SIMPLIFIED_SPECULATIVE_NUMBER_BINOP_LIST(DECLARE_CASE) #undef DECLARE_CASE @@ -461,17 +451,11 @@ class RepresentationSelector { default: // Shortcut for operations that we do not handle. if (type == nullptr) { - type = GetUpperBound(node); - info->set_unrestricted_feedback_type(type); - info->set_feedback_type( - Type::Intersect(type, info->restriction_type(), graph_zone())); + GetInfo(node)->set_feedback_type(NodeProperties::GetType(node)); return true; } return false; } - Type* unrestricted_feedback_type = new_type; - new_type = - Type::Intersect(new_type, info->restriction_type(), graph_zone()); // We need to guarantee that the feedback type is a subtype of the upper // bound. Naively that should hold, but weakening can actually produce // a bigger type if we are unlucky with ordering of phi typing. To be @@ -479,8 +463,7 @@ class RepresentationSelector { new_type = Type::Intersect(GetUpperBound(node), new_type, graph_zone()); if (type != nullptr && new_type->Is(type)) return false; - info->set_unrestricted_feedback_type(unrestricted_feedback_type); - info->set_feedback_type(new_type); + GetInfo(node)->set_feedback_type(new_type); if (FLAG_trace_representation) { PrintNodeFeedbackType(node); } @@ -1117,6 +1100,22 @@ class RepresentationSelector { return jsgraph_->simplified(); } + void LowerToCheckedInt32Mul(Node* node, Truncation truncation, + Type* input0_type, Type* input1_type) { + // If one of the inputs is positive and/or truncation is being applied, + // there is no need to return -0. + CheckForMinusZeroMode mz_mode = + truncation.IsUsedAsWord32() || + (input0_type->Is(Type::OrderedNumber()) && + input0_type->Min() > 0) || + (input1_type->Is(Type::OrderedNumber()) && + input1_type->Min() > 0) + ? CheckForMinusZeroMode::kDontCheckForMinusZero + : CheckForMinusZeroMode::kCheckForMinusZero; + + NodeProperties::ChangeOp(node, simplified()->CheckedInt32Mul(mz_mode)); + } + void ChangeToInt32OverflowOp(Node* node) { NodeProperties::ChangeOp(node, Int32OverflowOp(node)); } @@ -1144,32 +1143,29 @@ class RepresentationSelector { return; } - // We always take SignedSmall/Signed32 feedback otherwise. - NumberOperationHint const hint = NumberOperationHintOf(node->op()); - if (hint == NumberOperationHint::kSignedSmall || - hint == NumberOperationHint::kSigned32) { - // Check if we can guarantee truncations for the inputs. - if (BothInputsAre(node, Type::Signed32()) || - (BothInputsAre(node, Type::Signed32OrMinusZero()) && - NodeProperties::GetType(node)->Is(type_cache_.kSafeInteger))) { + // Try to use type feedback. + NumberOperationHint hint = NumberOperationHintOf(node->op()); + + // Handle the case when no int32 checks on inputs are necessary + // (but an overflow check is needed on the output). + if (BothInputsAre(node, Type::Signed32()) || + (BothInputsAre(node, Type::Signed32OrMinusZero()) && + NodeProperties::GetType(node)->Is(type_cache_.kSafeInteger))) { + // If both the inputs the feedback are int32, use the overflow op. + if (hint == NumberOperationHint::kSignedSmall || + hint == NumberOperationHint::kSigned32) { VisitBinop(node, UseInfo::TruncatingWord32(), MachineRepresentation::kWord32, Type::Signed32()); - } else { - VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint), - MachineRepresentation::kWord32, Type::Signed32()); - } - if (lower()) { - // Avoid overflow checks if the unrestricted feedback type of {node} - // suggests that the result will fit into Signed32/Unsigned32 range. - Type* const type = UnrestrictedFeedbackTypeOf(node); - if (type->Is(Type::Unsigned32())) { - ChangeToPureOp(node, Uint32Op(node)); - } else if (type->Is(Type::Signed32())) { - ChangeToPureOp(node, Int32Op(node)); - } else { - ChangeToInt32OverflowOp(node); - } + if (lower()) ChangeToInt32OverflowOp(node); + return; } + } + + if (hint == NumberOperationHint::kSignedSmall || + hint == NumberOperationHint::kSigned32) { + VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint), + MachineRepresentation::kWord32, Type::Signed32()); + if (lower()) ChangeToInt32OverflowOp(node); return; } @@ -1179,6 +1175,7 @@ class RepresentationSelector { if (lower()) { ChangeToPureOp(node, Float64Op(node)); } + return; } void VisitSpeculativeNumberModulus(Node* node, Truncation truncation, @@ -1520,39 +1517,33 @@ class RepresentationSelector { if (lower()) ChangeToPureOp(node, Int32Op(node)); return; } + // Try to use type feedback. + NumberOperationHint hint = NumberOperationHintOf(node->op()); + Type* input0_type = TypeOf(node->InputAt(0)); + Type* input1_type = TypeOf(node->InputAt(1)); - // We always take SignedSmall/Signed32 feedback otherwise. - NumberOperationHint const hint = NumberOperationHintOf(node->op()); - if (hint == NumberOperationHint::kSignedSmall || - hint == NumberOperationHint::kSigned32) { - // Handle the case when no int32 checks on inputs are necessary - // (but an overflow check is needed on the output). - if (BothInputsAre(node, Type::Signed32())) { - // If both the inputs the feedback are int32, use the overflow op. + // Handle the case when no int32 checks on inputs are necessary + // (but an overflow check is needed on the output). + if (BothInputsAre(node, Type::Signed32())) { + // If both the inputs the feedback are int32, use the overflow op. + if (hint == NumberOperationHint::kSignedSmall || + hint == NumberOperationHint::kSigned32) { VisitBinop(node, UseInfo::TruncatingWord32(), MachineRepresentation::kWord32, Type::Signed32()); - } else { - VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint), - MachineRepresentation::kWord32, Type::Signed32()); - } - if (lower()) { - // Avoid overflow checks if the unrestricted feedback type of {node} - // suggests that the result will fit into Signed32/Unsigned32 range, - // or at least try to avoid the expensive minus zero checks. - Type* const type = UnrestrictedFeedbackTypeOf(node); - if (type->Is(Type::Unsigned32())) { - ChangeToPureOp(node, Uint32Op(node)); - } else if (type->Is(Type::Signed32())) { - ChangeToPureOp(node, Int32Op(node)); - } else { - CheckForMinusZeroMode const mode = - (truncation.IdentifiesMinusZeroAndZero() || - !type->Maybe(Type::MinusZero())) - ? CheckForMinusZeroMode::kDontCheckForMinusZero - : CheckForMinusZeroMode::kCheckForMinusZero; - NodeProperties::ChangeOp(node, - simplified()->CheckedInt32Mul(mode)); + if (lower()) { + LowerToCheckedInt32Mul(node, truncation, input0_type, + input1_type); } + return; + } + } + + if (hint == NumberOperationHint::kSignedSmall || + hint == NumberOperationHint::kSigned32) { + VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint), + MachineRepresentation::kWord32, Type::Signed32()); + if (lower()) { + LowerToCheckedInt32Mul(node, truncation, input0_type, input1_type); } return; } diff --git a/test/mjsunit/regress/regress-5357.js b/test/mjsunit/regress/regress-5357.js new file mode 100644 index 0000000000..11ada60708 --- /dev/null +++ b/test/mjsunit/regress/regress-5357.js @@ -0,0 +1,17 @@ +// Copyright 2016 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: --allow-natives-syntax + +function foo(a) { + a++; + a = Math.max(0, a); + a++; + return a; +} + +foo(0); +foo(0); +%OptimizeFunctionOnNextCall(foo); +assertEquals(2147483648, foo(2147483646));