From ee158e6c4cc896479a32245432a3c2fdd31bcb73 Mon Sep 17 00:00:00 2001 From: bmeurer Date: Fri, 23 Sep 2016 12:26:53 -0700 Subject: [PATCH] [turbofan] ChangeFloat64ToTagged shouldn't canonicalize. This matches current Crankshaft/fullcodegen behavior more closely and thus reduces the chances that we run into unnecessary polymorphism due to the field representation tracking in our object model. R=jarin@chromium.org BUG=v8:5267 Committed: https://chromium.googlesource.com/v8/v8/+/6a939714e991ebf10d56ddbd2869325cca99c0ef Review-Url: https://codereview.chromium.org/2367593003 Cr-Commit-Position: refs/heads/master@{#39692} --- src/compiler/effect-control-linearizer.cc | 69 +------------------ src/compiler/representation-change.cc | 10 +-- src/compiler/simplified-operator.cc | 29 +------- src/compiler/simplified-operator.h | 2 +- src/js/regexp.js | 11 ++- src/runtime/runtime-test.cc | 4 +- .../simplified-operator-reducer-unittest.cc | 64 +++++++---------- 7 files changed, 43 insertions(+), 146 deletions(-) diff --git a/src/compiler/effect-control-linearizer.cc b/src/compiler/effect-control-linearizer.cc index 43d260153c..16892bebdb 100644 --- a/src/compiler/effect-control-linearizer.cc +++ b/src/compiler/effect-control-linearizer.cc @@ -776,75 +776,8 @@ bool EffectControlLinearizer::TryWireInStateEffect(Node* node, EffectControlLinearizer::ValueEffectControl EffectControlLinearizer::LowerChangeFloat64ToTagged(Node* node, Node* effect, Node* control) { - CheckForMinusZeroMode mode = CheckMinusZeroModeOf(node->op()); Node* value = node->InputAt(0); - - Node* value32 = graph()->NewNode(machine()->RoundFloat64ToInt32(), value); - Node* check_same = graph()->NewNode( - machine()->Float64Equal(), value, - graph()->NewNode(machine()->ChangeInt32ToFloat64(), value32)); - Node* branch_same = graph()->NewNode(common()->Branch(), check_same, control); - - Node* if_smi = graph()->NewNode(common()->IfTrue(), branch_same); - Node* vsmi; - Node* if_box = graph()->NewNode(common()->IfFalse(), branch_same); - - if (mode == CheckForMinusZeroMode::kCheckForMinusZero) { - // Check if {value} is -0. - Node* check_zero = graph()->NewNode(machine()->Word32Equal(), value32, - jsgraph()->Int32Constant(0)); - Node* branch_zero = graph()->NewNode(common()->Branch(BranchHint::kFalse), - check_zero, if_smi); - - Node* if_zero = graph()->NewNode(common()->IfTrue(), branch_zero); - Node* if_notzero = graph()->NewNode(common()->IfFalse(), branch_zero); - - // In case of 0, we need to check the high bits for the IEEE -0 pattern. - Node* check_negative = graph()->NewNode( - machine()->Int32LessThan(), - graph()->NewNode(machine()->Float64ExtractHighWord32(), value), - jsgraph()->Int32Constant(0)); - Node* branch_negative = graph()->NewNode( - common()->Branch(BranchHint::kFalse), check_negative, if_zero); - - Node* if_negative = graph()->NewNode(common()->IfTrue(), branch_negative); - Node* if_notnegative = - graph()->NewNode(common()->IfFalse(), branch_negative); - - // We need to create a box for negative 0. - if_smi = graph()->NewNode(common()->Merge(2), if_notzero, if_notnegative); - if_box = graph()->NewNode(common()->Merge(2), if_box, if_negative); - } - - // On 64-bit machines we can just wrap the 32-bit integer in a smi, for 32-bit - // machines we need to deal with potential overflow and fallback to boxing. - if (machine()->Is64()) { - vsmi = ChangeInt32ToSmi(value32); - } else { - Node* smi_tag = graph()->NewNode(machine()->Int32AddWithOverflow(), value32, - value32, if_smi); - - Node* check_ovf = - graph()->NewNode(common()->Projection(1), smi_tag, if_smi); - Node* branch_ovf = graph()->NewNode(common()->Branch(BranchHint::kFalse), - check_ovf, if_smi); - - Node* if_ovf = graph()->NewNode(common()->IfTrue(), branch_ovf); - if_box = graph()->NewNode(common()->Merge(2), if_ovf, if_box); - - if_smi = graph()->NewNode(common()->IfFalse(), branch_ovf); - vsmi = graph()->NewNode(common()->Projection(0), smi_tag, if_smi); - } - - // Allocate the box for the {value}. - ValueEffectControl box = AllocateHeapNumberWithValue(value, effect, if_box); - - control = graph()->NewNode(common()->Merge(2), if_smi, box.control); - value = graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2), - vsmi, box.value, control); - effect = - graph()->NewNode(common()->EffectPhi(2), effect, box.effect, control); - return ValueEffectControl(value, effect, control); + return AllocateHeapNumberWithValue(value, effect, control); } EffectControlLinearizer::ValueEffectControl diff --git a/src/compiler/representation-change.cc b/src/compiler/representation-change.cc index 97a2a37d59..22d809b9d6 100644 --- a/src/compiler/representation-change.cc +++ b/src/compiler/representation-change.cc @@ -366,10 +366,7 @@ Node* RepresentationChanger::GetTaggedRepresentationFor( } else if (output_rep == MachineRepresentation::kFloat32) { // float32 -> float64 -> tagged node = InsertChangeFloat32ToFloat64(node); - op = simplified()->ChangeFloat64ToTagged( - output_type->Maybe(Type::MinusZero()) - ? CheckForMinusZeroMode::kCheckForMinusZero - : CheckForMinusZeroMode::kDontCheckForMinusZero); + op = simplified()->ChangeFloat64ToTagged(); } else if (output_rep == MachineRepresentation::kFloat64) { if (output_type->Is(Type::Signed31())) { // float64 -> int32 -> tagged node = InsertChangeFloat64ToInt32(node); @@ -383,10 +380,7 @@ Node* RepresentationChanger::GetTaggedRepresentationFor( node = InsertChangeFloat64ToUint32(node); op = simplified()->ChangeUint32ToTagged(); } else { - op = simplified()->ChangeFloat64ToTagged( - output_type->Maybe(Type::MinusZero()) - ? CheckForMinusZeroMode::kCheckForMinusZero - : CheckForMinusZeroMode::kDontCheckForMinusZero); + op = simplified()->ChangeFloat64ToTagged(); } } else { return TypeError(node, output_rep, output_type, diff --git a/src/compiler/simplified-operator.cc b/src/compiler/simplified-operator.cc index ee03ba07bc..b7b230fc7b 100644 --- a/src/compiler/simplified-operator.cc +++ b/src/compiler/simplified-operator.cc @@ -208,8 +208,7 @@ CheckFloat64HoleMode CheckFloat64HoleModeOf(const Operator* op) { } CheckForMinusZeroMode CheckMinusZeroModeOf(const Operator* op) { - DCHECK(op->opcode() == IrOpcode::kChangeFloat64ToTagged || - op->opcode() == IrOpcode::kCheckedInt32Mul || + DCHECK(op->opcode() == IrOpcode::kCheckedInt32Mul || op->opcode() == IrOpcode::kCheckedFloat64ToInt32 || op->opcode() == IrOpcode::kCheckedTaggedToInt32); return OpParameter(op); @@ -399,6 +398,7 @@ PretenureFlag PretenureFlagOf(const Operator* op) { V(ChangeTaggedToInt32, Operator::kNoProperties, 1, 0) \ V(ChangeTaggedToUint32, Operator::kNoProperties, 1, 0) \ V(ChangeTaggedToFloat64, Operator::kNoProperties, 1, 0) \ + V(ChangeFloat64ToTagged, Operator::kNoProperties, 1, 0) \ V(ChangeInt31ToTaggedSigned, Operator::kNoProperties, 1, 0) \ V(ChangeInt32ToTagged, Operator::kNoProperties, 1, 0) \ V(ChangeUint32ToTagged, Operator::kNoProperties, 1, 0) \ @@ -475,19 +475,6 @@ struct SimplifiedOperatorGlobalCache final { }; ArrayBufferWasNeuteredOperator kArrayBufferWasNeutered; - template - struct ChangeFloat64ToTaggedOperator final - : public Operator1 { - ChangeFloat64ToTaggedOperator() - : Operator1( - IrOpcode::kChangeFloat64ToTagged, Operator::kPure, - "ChangeFloat64ToTagged", 1, 0, 0, 1, 0, 0, kMode) {} - }; - ChangeFloat64ToTaggedOperator - kChangeFloat64ToTaggedCheckForMinusZeroOperator; - ChangeFloat64ToTaggedOperator - kChangeFloat64ToTaggedDontCheckForMinusZeroOperator; - template struct CheckedInt32MulOperator final : public Operator1 { @@ -634,18 +621,6 @@ CHECKED_OP_LIST(GET_FROM_CACHE) GET_FROM_CACHE(ArrayBufferWasNeutered) #undef GET_FROM_CACHE -const Operator* SimplifiedOperatorBuilder::ChangeFloat64ToTagged( - CheckForMinusZeroMode mode) { - switch (mode) { - case CheckForMinusZeroMode::kCheckForMinusZero: - return &cache_.kChangeFloat64ToTaggedCheckForMinusZeroOperator; - case CheckForMinusZeroMode::kDontCheckForMinusZero: - return &cache_.kChangeFloat64ToTaggedDontCheckForMinusZeroOperator; - } - UNREACHABLE(); - return nullptr; -} - const Operator* SimplifiedOperatorBuilder::CheckedInt32Mul( CheckForMinusZeroMode mode) { switch (mode) { diff --git a/src/compiler/simplified-operator.h b/src/compiler/simplified-operator.h index ca2bf6d24b..8f1ecfc21e 100644 --- a/src/compiler/simplified-operator.h +++ b/src/compiler/simplified-operator.h @@ -301,7 +301,7 @@ class SimplifiedOperatorBuilder final : public ZoneObject { const Operator* ChangeInt31ToTaggedSigned(); const Operator* ChangeInt32ToTagged(); const Operator* ChangeUint32ToTagged(); - const Operator* ChangeFloat64ToTagged(CheckForMinusZeroMode); + const Operator* ChangeFloat64ToTagged(); const Operator* ChangeTaggedToBit(); const Operator* ChangeBitToTagged(); const Operator* TruncateTaggedToWord32(); diff --git a/src/js/regexp.js b/src/js/regexp.js index 396b694ea6..b3d28bab50 100644 --- a/src/js/regexp.js +++ b/src/js/regexp.js @@ -178,11 +178,20 @@ function RegExpExecJS(string) { var sticky = TO_BOOLEAN(REGEXP_STICKY(this)); var updateLastIndex = global || sticky; if (updateLastIndex) { - lastIndex = TO_LENGTH(this.lastIndex); + // TODO(jgruber): This is actually ToLength in the spec, but we bailout + // to the runtime in %_RegExpExec if lastIndex is not a Smi, so we are + // smart here and trick both TurboFan and Crankshaft to produce a Smi. + // This is a terrible hack, and correct for subtle reasons; it's a clear + // indicator that we need a predictable RegExp implementation where we + // don't need to add specific work-arounds for certain compiler issues. + lastIndex = +this.lastIndex; if (lastIndex > string.length) { this.lastIndex = 0; return null; + } else if (lastIndex <= 0) { + lastIndex = 0; } + lastIndex = lastIndex|0; } else { lastIndex = 0; } diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index d72ea6da68..8100d2c759 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -420,8 +420,8 @@ RUNTIME_FUNCTION(Runtime_SetAllocationTimeout) { SealHandleScope shs(isolate); DCHECK(args.length() == 2 || args.length() == 3); #ifdef DEBUG - CONVERT_SMI_ARG_CHECKED(interval, 0); - CONVERT_SMI_ARG_CHECKED(timeout, 1); + CONVERT_INT32_ARG_CHECKED(interval, 0); + CONVERT_INT32_ARG_CHECKED(timeout, 1); isolate->heap()->set_allocation_timeout(timeout); FLAG_gc_interval = interval; if (args.length() == 3) { diff --git a/test/unittests/compiler/simplified-operator-reducer-unittest.cc b/test/unittests/compiler/simplified-operator-reducer-unittest.cc index d50d2ff4ef..6f37609f3a 100644 --- a/test/unittests/compiler/simplified-operator-reducer-unittest.cc +++ b/test/unittests/compiler/simplified-operator-reducer-unittest.cc @@ -97,10 +97,6 @@ const double kNaNs[] = {-std::numeric_limits::quiet_NaN(), bit_cast(V8_UINT64_C(0x7FFFFFFFFFFFFFFF)), bit_cast(V8_UINT64_C(0xFFFFFFFFFFFFFFFF))}; -const CheckForMinusZeroMode kCheckForMinusZeroModes[] = { - CheckForMinusZeroMode::kDontCheckForMinusZero, - CheckForMinusZeroMode::kCheckForMinusZero}; - } // namespace @@ -191,13 +187,11 @@ TEST_F(SimplifiedOperatorReducerTest, ChangeTaggedToBitWithChangeBitToTagged) { // ChangeFloat64ToTagged TEST_F(SimplifiedOperatorReducerTest, ChangeFloat64ToTaggedWithConstant) { - TRACED_FOREACH(CheckForMinusZeroMode, mode, kCheckForMinusZeroModes) { - TRACED_FOREACH(double, n, kFloat64Values) { - Reduction reduction = Reduce(graph()->NewNode( - simplified()->ChangeFloat64ToTagged(mode), Float64Constant(n))); - ASSERT_TRUE(reduction.Changed()); - EXPECT_THAT(reduction.replacement(), IsNumberConstant(BitEq(n))); - } + TRACED_FOREACH(double, n, kFloat64Values) { + Reduction reduction = Reduce(graph()->NewNode( + simplified()->ChangeFloat64ToTagged(), Float64Constant(n))); + ASSERT_TRUE(reduction.Changed()); + EXPECT_THAT(reduction.replacement(), IsNumberConstant(BitEq(n))); } } @@ -222,13 +216,11 @@ TEST_F(SimplifiedOperatorReducerTest, ChangeInt32ToTaggedWithConstant) { TEST_F(SimplifiedOperatorReducerTest, ChangeTaggedToFloat64WithChangeFloat64ToTagged) { Node* param0 = Parameter(0); - TRACED_FOREACH(CheckForMinusZeroMode, mode, kCheckForMinusZeroModes) { - Reduction reduction = Reduce(graph()->NewNode( - simplified()->ChangeTaggedToFloat64(), - graph()->NewNode(simplified()->ChangeFloat64ToTagged(mode), param0))); - ASSERT_TRUE(reduction.Changed()); - EXPECT_EQ(param0, reduction.replacement()); - } + Reduction reduction = Reduce(graph()->NewNode( + simplified()->ChangeTaggedToFloat64(), + graph()->NewNode(simplified()->ChangeFloat64ToTagged(), param0))); + ASSERT_TRUE(reduction.Changed()); + EXPECT_EQ(param0, reduction.replacement()); } TEST_F(SimplifiedOperatorReducerTest, @@ -279,13 +271,11 @@ TEST_F(SimplifiedOperatorReducerTest, ChangeTaggedToFloat64WithNaNConstant) { TEST_F(SimplifiedOperatorReducerTest, ChangeTaggedToInt32WithChangeFloat64ToTagged) { Node* param0 = Parameter(0); - TRACED_FOREACH(CheckForMinusZeroMode, mode, kCheckForMinusZeroModes) { - Reduction reduction = Reduce(graph()->NewNode( - simplified()->ChangeTaggedToInt32(), - graph()->NewNode(simplified()->ChangeFloat64ToTagged(mode), param0))); - ASSERT_TRUE(reduction.Changed()); - EXPECT_THAT(reduction.replacement(), IsChangeFloat64ToInt32(param0)); - } + Reduction reduction = Reduce(graph()->NewNode( + simplified()->ChangeTaggedToInt32(), + graph()->NewNode(simplified()->ChangeFloat64ToTagged(), param0))); + ASSERT_TRUE(reduction.Changed()); + EXPECT_THAT(reduction.replacement(), IsChangeFloat64ToInt32(param0)); } TEST_F(SimplifiedOperatorReducerTest, @@ -305,13 +295,11 @@ TEST_F(SimplifiedOperatorReducerTest, TEST_F(SimplifiedOperatorReducerTest, ChangeTaggedToUint32WithChangeFloat64ToTagged) { Node* param0 = Parameter(0); - TRACED_FOREACH(CheckForMinusZeroMode, mode, kCheckForMinusZeroModes) { - Reduction reduction = Reduce(graph()->NewNode( - simplified()->ChangeTaggedToUint32(), - graph()->NewNode(simplified()->ChangeFloat64ToTagged(mode), param0))); - ASSERT_TRUE(reduction.Changed()); - EXPECT_THAT(reduction.replacement(), IsChangeFloat64ToUint32(param0)); - } + Reduction reduction = Reduce(graph()->NewNode( + simplified()->ChangeTaggedToUint32(), + graph()->NewNode(simplified()->ChangeFloat64ToTagged(), param0))); + ASSERT_TRUE(reduction.Changed()); + EXPECT_THAT(reduction.replacement(), IsChangeFloat64ToUint32(param0)); } TEST_F(SimplifiedOperatorReducerTest, @@ -331,13 +319,11 @@ TEST_F(SimplifiedOperatorReducerTest, TEST_F(SimplifiedOperatorReducerTest, TruncateTaggedToWord3WithChangeFloat64ToTagged) { Node* param0 = Parameter(0); - TRACED_FOREACH(CheckForMinusZeroMode, mode, kCheckForMinusZeroModes) { - Reduction reduction = Reduce(graph()->NewNode( - simplified()->TruncateTaggedToWord32(), - graph()->NewNode(simplified()->ChangeFloat64ToTagged(mode), param0))); - ASSERT_TRUE(reduction.Changed()); - EXPECT_THAT(reduction.replacement(), IsTruncateFloat64ToWord32(param0)); - } + Reduction reduction = Reduce(graph()->NewNode( + simplified()->TruncateTaggedToWord32(), + graph()->NewNode(simplified()->ChangeFloat64ToTagged(), param0))); + ASSERT_TRUE(reduction.Changed()); + EXPECT_THAT(reduction.replacement(), IsTruncateFloat64ToWord32(param0)); } TEST_F(SimplifiedOperatorReducerTest, TruncateTaggedToWord32WithConstant) {