From e7111cfff7d73786aeb5d7ed579103137f532748 Mon Sep 17 00:00:00 2001 From: mvstanton Date: Thu, 14 Jul 2016 06:21:55 -0700 Subject: [PATCH] [Turbofan]: Add integer multiplication with overflow to typed lowering. BUG= Review-Url: https://codereview.chromium.org/2141953002 Cr-Commit-Position: refs/heads/master@{#37764} --- src/code-stub-assembler.cc | 61 +++++++++++++++++++++++ src/code-stub-assembler.h | 2 + src/code-stubs.cc | 54 ++------------------ src/compiler/effect-control-linearizer.cc | 44 ++++++++++++++++ src/compiler/effect-control-linearizer.h | 2 + src/compiler/js-graph.cc | 1 - src/compiler/js-typed-lowering.cc | 8 +++ src/compiler/opcodes.h | 1 + src/compiler/redundancy-elimination.cc | 1 + src/compiler/representation-change.cc | 2 + src/compiler/simplified-lowering.cc | 57 +++++++++++++++++---- src/compiler/simplified-operator.cc | 1 + src/compiler/simplified-operator.h | 1 + src/compiler/verifier.cc | 1 + test/mjsunit/math-mul.js | 10 +++- test/mjsunit/mjsunit.status | 5 +- 16 files changed, 188 insertions(+), 63 deletions(-) diff --git a/src/code-stub-assembler.cc b/src/code-stub-assembler.cc index 3aa27ecbd3..eb7e49da46 100644 --- a/src/code-stub-assembler.cc +++ b/src/code-stub-assembler.cc @@ -401,6 +401,67 @@ Node* CodeStubAssembler::SmiMod(Node* a, Node* b) { return var_result.value(); } +Node* CodeStubAssembler::SmiMul(Node* a, Node* b) { + Variable var_result(this, MachineRepresentation::kTagged); + Variable var_lhs_float64(this, MachineRepresentation::kFloat64), + var_rhs_float64(this, MachineRepresentation::kFloat64); + Label return_result(this, &var_result); + + // Both {a} and {b} are Smis. Convert them to integers and multiply. + Node* lhs32 = SmiToWord32(a); + Node* rhs32 = SmiToWord32(b); + Node* pair = Int32MulWithOverflow(lhs32, rhs32); + + Node* overflow = Projection(1, pair); + + // Check if the multiplication overflowed. + Label if_overflow(this, Label::kDeferred), if_notoverflow(this); + Branch(overflow, &if_overflow, &if_notoverflow); + Bind(&if_notoverflow); + { + // If the answer is zero, we may need to return -0.0, depending on the + // input. + Label answer_zero(this), answer_not_zero(this); + Node* answer = Projection(0, pair); + Node* zero = Int32Constant(0); + Branch(WordEqual(answer, zero), &answer_zero, &answer_not_zero); + Bind(&answer_not_zero); + { + var_result.Bind(ChangeInt32ToTagged(answer)); + Goto(&return_result); + } + Bind(&answer_zero); + { + Node* or_result = Word32Or(lhs32, rhs32); + Label if_should_be_negative_zero(this), if_should_be_zero(this); + Branch(Int32LessThan(or_result, zero), &if_should_be_negative_zero, + &if_should_be_zero); + Bind(&if_should_be_negative_zero); + { + var_result.Bind(MinusZeroConstant()); + Goto(&return_result); + } + Bind(&if_should_be_zero); + { + var_result.Bind(zero); + Goto(&return_result); + } + } + } + Bind(&if_overflow); + { + var_lhs_float64.Bind(SmiToFloat64(a)); + var_rhs_float64.Bind(SmiToFloat64(b)); + Node* value = Float64Mul(var_lhs_float64.value(), var_rhs_float64.value()); + Node* result = ChangeFloat64ToTagged(value); + var_result.Bind(result); + Goto(&return_result); + } + + Bind(&return_result); + return var_result.value(); +} + Node* CodeStubAssembler::WordIsSmi(Node* a) { return WordEqual(WordAnd(a, IntPtrConstant(kSmiTagMask)), IntPtrConstant(0)); } diff --git a/src/code-stub-assembler.h b/src/code-stub-assembler.h index 814e0d4be8..e3c4a13c23 100644 --- a/src/code-stub-assembler.h +++ b/src/code-stub-assembler.h @@ -82,6 +82,8 @@ class CodeStubAssembler : public compiler::CodeAssembler { compiler::Node* SmiMin(compiler::Node* a, compiler::Node* b); // Computes a % b for Smi inputs a and b; result is not necessarily a Smi. compiler::Node* SmiMod(compiler::Node* a, compiler::Node* b); + // Computes a * b for Smi inputs a and b; result is not necessarily a Smi. + compiler::Node* SmiMul(compiler::Node* a, compiler::Node* b); // Allocate an object of the given size. compiler::Node* Allocate(compiler::Node* size, AllocationFlags flags = kNone); diff --git a/src/code-stubs.cc b/src/code-stubs.cc index 6ee52877ca..d78b0773c2 100644 --- a/src/code-stubs.cc +++ b/src/code-stubs.cc @@ -1097,56 +1097,10 @@ compiler::Node* MultiplyStub::Generate(CodeStubAssembler* assembler, assembler->Bind(&rhs_is_smi); { - // Both {lhs} and {rhs} are Smis. Convert them to integers and multiply. - Node* lhs32 = assembler->SmiToWord32(lhs); - Node* rhs32 = assembler->SmiToWord32(rhs); - Node* pair = assembler->Int32MulWithOverflow(lhs32, rhs32); - - Node* overflow = assembler->Projection(1, pair); - - // Check if the multiplication overflowed. - Label if_overflow(assembler, Label::kDeferred), - if_notoverflow(assembler); - assembler->Branch(overflow, &if_overflow, &if_notoverflow); - assembler->Bind(&if_notoverflow); - { - // If the answer is zero, we may need to return -0.0, depending on the - // input. - Label answer_zero(assembler), answer_not_zero(assembler); - Node* answer = assembler->Projection(0, pair); - Node* zero = assembler->Int32Constant(0); - assembler->Branch(assembler->WordEqual(answer, zero), &answer_zero, - &answer_not_zero); - assembler->Bind(&answer_not_zero); - { - var_result.Bind(assembler->ChangeInt32ToTagged(answer)); - assembler->Goto(&return_result); - } - assembler->Bind(&answer_zero); - { - Node* or_result = assembler->Word32Or(lhs32, rhs32); - Label if_should_be_negative_zero(assembler), - if_should_be_zero(assembler); - assembler->Branch(assembler->Int32LessThan(or_result, zero), - &if_should_be_negative_zero, &if_should_be_zero); - assembler->Bind(&if_should_be_negative_zero); - { - var_result.Bind(assembler->MinusZeroConstant()); - assembler->Goto(&return_result); - } - assembler->Bind(&if_should_be_zero); - { - var_result.Bind(zero); - assembler->Goto(&return_result); - } - } - } - assembler->Bind(&if_overflow); - { - var_lhs_float64.Bind(assembler->SmiToFloat64(lhs)); - var_rhs_float64.Bind(assembler->SmiToFloat64(rhs)); - assembler->Goto(&do_fmul); - } + // Both {lhs} and {rhs} are Smis. The result is not necessarily a smi, + // in case of overflow. + var_result.Bind(assembler->SmiMul(lhs, rhs)); + assembler->Goto(&return_result); } assembler->Bind(&rhs_is_not_smi); diff --git a/src/compiler/effect-control-linearizer.cc b/src/compiler/effect-control-linearizer.cc index 65a8a6527e..292d4d58e2 100644 --- a/src/compiler/effect-control-linearizer.cc +++ b/src/compiler/effect-control-linearizer.cc @@ -648,6 +648,9 @@ bool EffectControlLinearizer::TryWireInStateEffect(Node* node, case IrOpcode::kCheckedUint32Mod: state = LowerCheckedUint32Mod(node, frame_state, *effect, *control); break; + case IrOpcode::kCheckedInt32Mul: + state = LowerCheckedInt32Mul(node, frame_state, *effect, *control); + break; case IrOpcode::kCheckedUint32ToInt32: state = LowerCheckedUint32ToInt32(node, frame_state, *effect, *control); break; @@ -1298,6 +1301,47 @@ EffectControlLinearizer::LowerCheckedUint32Mod(Node* node, Node* frame_state, return ValueEffectControl(value, effect, control); } +EffectControlLinearizer::ValueEffectControl +EffectControlLinearizer::LowerCheckedInt32Mul(Node* node, Node* frame_state, + Node* effect, Node* control) { + Node* zero = jsgraph()->Int32Constant(0); + Node* lhs = node->InputAt(0); + Node* rhs = node->InputAt(1); + + Node* projection = + graph()->NewNode(machine()->Int32MulWithOverflow(), lhs, rhs, control); + + Node* check = graph()->NewNode(common()->Projection(1), projection, control); + control = effect = graph()->NewNode(common()->DeoptimizeIf(), check, + frame_state, effect, control); + + Node* value = graph()->NewNode(common()->Projection(0), projection, control); + + Node* check_zero = graph()->NewNode(machine()->Word32Equal(), value, zero); + Node* branch_zero = graph()->NewNode(common()->Branch(BranchHint::kFalse), + check_zero, control); + + Node* if_zero = graph()->NewNode(common()->IfTrue(), branch_zero); + Node* e_if_zero = effect; + { + // We may need to return negative zero. + Node* or_inputs = graph()->NewNode(machine()->Word32Or(), lhs, rhs); + Node* check_or = + graph()->NewNode(machine()->Int32LessThan(), or_inputs, zero); + if_zero = e_if_zero = graph()->NewNode(common()->DeoptimizeIf(), check_or, + frame_state, e_if_zero, if_zero); + } + + Node* if_not_zero = graph()->NewNode(common()->IfFalse(), branch_zero); + Node* e_if_not_zero = effect; + + control = graph()->NewNode(common()->Merge(2), if_zero, if_not_zero); + effect = graph()->NewNode(common()->EffectPhi(2), e_if_zero, e_if_not_zero, + control); + + return ValueEffectControl(value, effect, control); +} + EffectControlLinearizer::ValueEffectControl EffectControlLinearizer::LowerCheckedUint32ToInt32(Node* node, Node* frame_state, diff --git a/src/compiler/effect-control-linearizer.h b/src/compiler/effect-control-linearizer.h index 318a5a212b..5b59bd71aa 100644 --- a/src/compiler/effect-control-linearizer.h +++ b/src/compiler/effect-control-linearizer.h @@ -83,6 +83,8 @@ class EffectControlLinearizer { Node* effect, Node* control); ValueEffectControl LowerCheckedUint32Mod(Node* node, Node* frame_state, Node* effect, Node* control); + ValueEffectControl LowerCheckedInt32Mul(Node* node, Node* frame_state, + Node* effect, Node* control); ValueEffectControl LowerCheckedUint32ToInt32(Node* node, Node* frame_state, Node* effect, Node* control); ValueEffectControl LowerCheckedFloat64ToInt32(Node* node, Node* frame_state, diff --git a/src/compiler/js-graph.cc b/src/compiler/js-graph.cc index 3f20daa3d1..82c22c3719 100644 --- a/src/compiler/js-graph.cc +++ b/src/compiler/js-graph.cc @@ -92,7 +92,6 @@ Node* JSGraph::ZeroConstant() { return CACHED(kZeroConstant, NumberConstant(0.0)); } - Node* JSGraph::OneConstant() { return CACHED(kOneConstant, NumberConstant(1.0)); } diff --git a/src/compiler/js-typed-lowering.cc b/src/compiler/js-typed-lowering.cc index 396e15b044..27271fcdb3 100644 --- a/src/compiler/js-typed-lowering.cc +++ b/src/compiler/js-typed-lowering.cc @@ -510,6 +510,14 @@ Reduction JSTypedLowering::ReduceJSSubtract(Node* node) { Reduction JSTypedLowering::ReduceJSMultiply(Node* node) { JSBinopReduction r(this, node); BinaryOperationHints::Hint feedback = r.GetNumberBinaryOperationFeedback(); + if (feedback == BinaryOperationHints::kNumberOrOddball && + r.BothInputsAre(Type::PlainPrimitive())) { + // JSMultiply(x:plain-primitive, + // y:plain-primitive) => NumberMultiply(ToNumber(x), ToNumber(y)) + r.ConvertInputsToNumber(); + return r.ChangeToPureOperator(simplified()->NumberMultiply(), + Type::Number()); + } if (feedback != BinaryOperationHints::kAny) { return r.ChangeToSpeculativeOperator( simplified()->SpeculativeNumberMultiply(feedback), Type::Number()); diff --git a/src/compiler/opcodes.h b/src/compiler/opcodes.h index a5f0a5da5b..f2cf595ebc 100644 --- a/src/compiler/opcodes.h +++ b/src/compiler/opcodes.h @@ -184,6 +184,7 @@ V(CheckedInt32Mod) \ V(CheckedUint32Div) \ V(CheckedUint32Mod) \ + V(CheckedInt32Mul) \ V(CheckedUint32ToInt32) \ V(CheckedFloat64ToInt32) \ V(CheckedTaggedToInt32) \ diff --git a/src/compiler/redundancy-elimination.cc b/src/compiler/redundancy-elimination.cc index 0ef6777d37..4c50b19fa5 100644 --- a/src/compiler/redundancy-elimination.cc +++ b/src/compiler/redundancy-elimination.cc @@ -29,6 +29,7 @@ Reduction RedundancyElimination::Reduce(Node* node) { case IrOpcode::kCheckedInt32Sub: case IrOpcode::kCheckedInt32Div: case IrOpcode::kCheckedInt32Mod: + case IrOpcode::kCheckedInt32Mul: case IrOpcode::kCheckedTaggedToFloat64: case IrOpcode::kCheckedTaggedToInt32: case IrOpcode::kCheckedUint32ToInt32: diff --git a/src/compiler/representation-change.cc b/src/compiler/representation-change.cc index ba75a3388f..945cb1825d 100644 --- a/src/compiler/representation-change.cc +++ b/src/compiler/representation-change.cc @@ -627,6 +627,8 @@ const Operator* RepresentationChanger::Int32OverflowOperatorFor( return simplified()->CheckedInt32Div(); case IrOpcode::kSpeculativeNumberModulus: return simplified()->CheckedInt32Mod(); + case IrOpcode::kSpeculativeNumberMultiply: + return simplified()->CheckedInt32Mul(); default: UNREACHABLE(); return nullptr; diff --git a/src/compiler/simplified-lowering.cc b/src/compiler/simplified-lowering.cc index eebef1c935..ad4a3a24e3 100644 --- a/src/compiler/simplified-lowering.cc +++ b/src/compiler/simplified-lowering.cc @@ -1311,7 +1311,52 @@ class RepresentationSelector { } return; } - case IrOpcode::kSpeculativeNumberMultiply: + case IrOpcode::kSpeculativeNumberMultiply: { + if (BothInputsAre(node, Type::Integral32()) && + (NodeProperties::GetType(node)->Is(Type::Signed32()) || + NodeProperties::GetType(node)->Is(Type::Unsigned32()) || + (truncation.TruncatesToWord32() && + NodeProperties::GetType(node)->Is( + type_cache_.kSafeIntegerOrMinusZero)))) { + // Multiply reduces to Int32Mul if the inputs are integers, and + // (a) the output is either known to be Signed32, or + // (b) the output is known to be Unsigned32, or + // (c) the uses are truncating and the result is in the safe + // integer range. + VisitWord32TruncatingBinop(node); + if (lower()) ChangeToPureOp(node, Int32Op(node)); + return; + } + // Try to use type feedback. + BinaryOperationHints::Hint hint = BinaryOperationHintOf(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())) { + // If both the inputs the feedback are int32, use the overflow op. + if (hint == BinaryOperationHints::kSignedSmall || + hint == BinaryOperationHints::kSigned32) { + VisitBinop(node, UseInfo::TruncatingWord32(), + MachineRepresentation::kWord32, Type::Signed32()); + if (lower()) ChangeToInt32OverflowOp(node); + return; + } + } + + if (hint == BinaryOperationHints::kSignedSmall || + hint == BinaryOperationHints::kSigned32) { + VisitBinop(node, UseInfo::CheckedSigned32AsWord32(), + MachineRepresentation::kWord32, Type::Signed32()); + if (lower()) ChangeToInt32OverflowOp(node); + return; + } + + // Checked float64 x float64 => float64 + VisitBinop(node, UseInfo::CheckedNumberOrOddballAsFloat64(), + MachineRepresentation::kFloat64, Type::Number()); + if (lower()) ChangeToPureOp(node, Float64Op(node)); + return; + } case IrOpcode::kNumberMultiply: { if (BothInputsAre(node, Type::Integral32()) && (NodeProperties::GetType(node)->Is(Type::Signed32()) || @@ -1329,15 +1374,7 @@ class RepresentationSelector { return; } // Number x Number => Float64Mul - if (BothInputsAre(node, Type::NumberOrUndefined())) { - VisitFloat64Binop(node); - if (lower()) ChangeToPureOp(node, Float64Op(node)); - return; - } - // Checked float64 x float64 => float64 - DCHECK_EQ(IrOpcode::kSpeculativeNumberMultiply, node->opcode()); - VisitBinop(node, UseInfo::CheckedNumberOrOddballAsFloat64(), - MachineRepresentation::kFloat64, Type::Number()); + VisitFloat64Binop(node); if (lower()) ChangeToPureOp(node, Float64Op(node)); return; } diff --git a/src/compiler/simplified-operator.cc b/src/compiler/simplified-operator.cc index 5c73b593e0..c84475e7ac 100644 --- a/src/compiler/simplified-operator.cc +++ b/src/compiler/simplified-operator.cc @@ -325,6 +325,7 @@ CompareOperationHints::Hint CompareOperationHintOf(const Operator* op) { V(CheckedInt32Mod, 2, 1) \ V(CheckedUint32Div, 2, 1) \ V(CheckedUint32Mod, 2, 1) \ + V(CheckedInt32Mul, 2, 1) \ V(CheckedUint32ToInt32, 1, 1) \ V(CheckedFloat64ToInt32, 1, 1) \ V(CheckedTaggedToInt32, 1, 1) \ diff --git a/src/compiler/simplified-operator.h b/src/compiler/simplified-operator.h index 011d239cd8..4058e509e8 100644 --- a/src/compiler/simplified-operator.h +++ b/src/compiler/simplified-operator.h @@ -257,6 +257,7 @@ class SimplifiedOperatorBuilder final : public ZoneObject { const Operator* CheckedInt32Mod(); const Operator* CheckedUint32Div(); const Operator* CheckedUint32Mod(); + const Operator* CheckedInt32Mul(); const Operator* CheckedUint32ToInt32(); const Operator* CheckedFloat64ToInt32(); const Operator* CheckedTaggedToInt32(); diff --git a/src/compiler/verifier.cc b/src/compiler/verifier.cc index 55c414bf85..40746437bb 100644 --- a/src/compiler/verifier.cc +++ b/src/compiler/verifier.cc @@ -951,6 +951,7 @@ void Verifier::Visitor::Check(Node* node) { case IrOpcode::kCheckedInt32Mod: case IrOpcode::kCheckedUint32Div: case IrOpcode::kCheckedUint32Mod: + case IrOpcode::kCheckedInt32Mul: case IrOpcode::kCheckedUint32ToInt32: case IrOpcode::kCheckedFloat64ToInt32: case IrOpcode::kCheckedTaggedToInt32: diff --git a/test/mjsunit/math-mul.js b/test/mjsunit/math-mul.js index 855c32f8a4..027bb6024a 100644 --- a/test/mjsunit/math-mul.js +++ b/test/mjsunit/math-mul.js @@ -2,12 +2,20 @@ // 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 test(x, y) { return x * y; } -assertEquals(8, test(2, 4)); +assertEquals(12, test(3, 4)); +assertEquals(16, test(4, 4)); + +%OptimizeFunctionOnNextCall(test); +assertEquals(27, test(9, 3)); + assertEquals(-0, test(-3, 0)); assertEquals(-0, test(0, -0)); + const SMI_MAX = (1 << 29) - 1 + (1 << 29); // Create without overflowing. const SMI_MIN = -SMI_MAX - 1; // Create without overflowing. diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index c04db1025d..b707689148 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -254,7 +254,10 @@ 'getters-on-elements': [PASS, NO_IGNITION], 'harmony/do-expressions': [PASS, NO_IGNITION], 'math-floor-of-div-minus-zero': [PASS, NO_IGNITION], - 'regress/regress-2132': [PASS, NO_IGNITION], + + # TODO(mvstanton): restore NO_IGNITION. + 'regress/regress-2132': [PASS, NO_VARIANTS], + 'regress/regress-2339': [PASS, NO_IGNITION], 'regress/regress-3176': [PASS, NO_IGNITION], 'regress/regress-3709': [PASS, NO_IGNITION],