From 8f7f26c0a4dab0e30faf8129fd5e0f27f4b0f164 Mon Sep 17 00:00:00 2001 From: "vegorov@google.com" Date: Fri, 14 Nov 2014 11:19:42 +0000 Subject: [PATCH] Reland "Suppress canonicalization of Unbox() instruction that can deoptimize." This relands commit r41693 with additional fixes in canonicalization pass. R=fschneider@google.com BUG= Review URL: https://codereview.chromium.org//726593002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@41736 260f80e4-7a28-3924-810f-c04153c831b5 --- runtime/vm/flow_graph_optimizer.cc | 7 +++ runtime/vm/flow_graph_range_analysis.cc | 48 +++++++-------- runtime/vm/flow_graph_range_analysis.h | 4 +- runtime/vm/flow_graph_range_analysis_test.cc | 5 +- runtime/vm/intermediate_language.cc | 58 ++++++++++--------- runtime/vm/intermediate_language.h | 2 + ...canonicalization_preserves_deopt_test.dart | 41 +++++++++++++ 7 files changed, 111 insertions(+), 54 deletions(-) create mode 100644 tests/language/vm/canonicalization_preserves_deopt_test.dart diff --git a/runtime/vm/flow_graph_optimizer.cc b/runtime/vm/flow_graph_optimizer.cc index 3156dccc662..3d2d5479bd0 100644 --- a/runtime/vm/flow_graph_optimizer.cc +++ b/runtime/vm/flow_graph_optimizer.cc @@ -589,7 +589,14 @@ bool FlowGraphOptimizer::Canonicalize() { BlockEntryInstr* entry = block_order_[i]; for (ForwardInstructionIterator it(entry); !it.Done(); it.Advance()) { Instruction* current = it.Current(); + if (current->HasUnmatchedInputRepresentations()) { + // Can't canonicalize this instruction until all conversions for its + // inputs are inserted. + continue; + } + Instruction* replacement = current->Canonicalize(flow_graph()); + if (replacement != current) { // For non-definitions Canonicalize should return either NULL or // this. diff --git a/runtime/vm/flow_graph_range_analysis.cc b/runtime/vm/flow_graph_range_analysis.cc index 0f875339635..d4f192a4c80 100644 --- a/runtime/vm/flow_graph_range_analysis.cc +++ b/runtime/vm/flow_graph_range_analysis.cc @@ -2402,7 +2402,7 @@ void Range::Shr(const Range* left, } -bool Range::And(const Range* left_range, +void Range::And(const Range* left_range, const Range* right_range, RangeBoundary* result_min, RangeBoundary* result_max) { @@ -2414,16 +2414,17 @@ bool Range::And(const Range* left_range, if (Range::ConstantMin(right_range).ConstantValue() >= 0) { *result_min = RangeBoundary::FromConstant(0); *result_max = Range::ConstantMax(right_range); - return true; + return; } if (Range::ConstantMin(left_range).ConstantValue() >= 0) { *result_min = RangeBoundary::FromConstant(0); *result_max = Range::ConstantMax(left_range); - return true; + return; } - return false; + *result_min = RangeBoundary::MinConstant(RangeBoundary::kRangeBoundaryInt64); + *result_max = RangeBoundary::MaxConstant(RangeBoundary::kRangeBoundaryInt64); } @@ -2524,7 +2525,7 @@ void Range::Sub(const Range* left_range, } -bool Range::Mul(const Range* left_range, +void Range::Mul(const Range* left_range, const Range* right_range, RangeBoundary* result_min, RangeBoundary* result_max) { @@ -2545,7 +2546,7 @@ bool Range::Mul(const Range* left_range, const int64_t r_max = OnlyNegativeOrZero(*left_range, *right_range) ? 0 : mul_max; *result_max = RangeBoundary::FromConstant(r_max); - return true; + return; } // TODO(vegorov): handle mixed sign case that leads to (-Infinity, 0] range. @@ -2553,10 +2554,11 @@ bool Range::Mul(const Range* left_range, OnlyNegativeOrZero(*left_range, *right_range)) { *result_min = RangeBoundary::FromConstant(0); *result_max = RangeBoundary::PositiveInfinity(); - return true; + return; } - return false; + *result_min = RangeBoundary::NegativeInfinity(); + *result_max = RangeBoundary::PositiveInfinity(); } @@ -2603,39 +2605,39 @@ void Range::BinaryOp(const Token::Kind op, case Token::kADD: Range::Add(left_range, right_range, &min, &max, left_defn); break; + case Token::kSUB: Range::Sub(left_range, right_range, &min, &max, left_defn); break; - case Token::kMUL: { - if (!Range::Mul(left_range, right_range, &min, &max)) { - *result = Range::Full(RangeBoundary::kRangeBoundaryInt64); - return; - } + + case Token::kMUL: + Range::Mul(left_range, right_range, &min, &max); break; - } - case Token::kSHL: { + + case Token::kSHL: Range::Shl(left_range, right_range, &min, &max); break; - } - case Token::kSHR: { + + case Token::kSHR: Range::Shr(left_range, right_range, &min, &max); break; - } case Token::kBIT_AND: - if (!Range::And(left_range, right_range, &min, &max)) { - *result = Range::Full(RangeBoundary::kRangeBoundaryInt64); - return; - } + Range::And(left_range, right_range, &min, &max); break; case Token::kBIT_XOR: Range::Xor(left_range, right_range, &min, &max); break; - default: + case Token::kBIT_OR: *result = Range::Full(RangeBoundary::kRangeBoundaryInt64); return; + + default: + *result = Range(RangeBoundary::NegativeInfinity(), + RangeBoundary::PositiveInfinity()); + return; } ASSERT(!min.IsUnknown() && !max.IsUnknown()); diff --git a/runtime/vm/flow_graph_range_analysis.h b/runtime/vm/flow_graph_range_analysis.h index a4bc7e6cf6f..3b39ba2a45e 100644 --- a/runtime/vm/flow_graph_range_analysis.h +++ b/runtime/vm/flow_graph_range_analysis.h @@ -440,7 +440,7 @@ class Range : public ZoneAllocated { RangeBoundary* max, Definition* left_defn); - static bool Mul(const Range* left_range, + static void Mul(const Range* left_range, const Range* right_range, RangeBoundary* min, RangeBoundary* max); @@ -454,7 +454,7 @@ class Range : public ZoneAllocated { RangeBoundary* min, RangeBoundary* max); - static bool And(const Range* left_range, + static void And(const Range* left_range, const Range* right_range, RangeBoundary* min, RangeBoundary* max); diff --git a/runtime/vm/flow_graph_range_analysis_test.cc b/runtime/vm/flow_graph_range_analysis_test.cc index 781aae24f6c..4b38b44c213 100644 --- a/runtime/vm/flow_graph_range_analysis_test.cc +++ b/runtime/vm/flow_graph_range_analysis_test.cc @@ -531,8 +531,9 @@ TEST_CASE(RangeAnd) { static_cast(20), static_cast(-20), static_cast(20), - RangeBoundary(), - RangeBoundary()); + RangeBoundary::MinConstant(RangeBoundary::kRangeBoundaryInt64), + RangeBoundary::MaxConstant( + RangeBoundary::kRangeBoundaryInt64)); #undef TEST_RANGE_AND } diff --git a/runtime/vm/intermediate_language.cc b/runtime/vm/intermediate_language.cc index 59f0cb232b2..8040c5063a4 100644 --- a/runtime/vm/intermediate_language.cc +++ b/runtime/vm/intermediate_language.cc @@ -789,6 +789,18 @@ bool Instruction::IsDominatedBy(Instruction* dom) { } +bool Instruction::HasUnmatchedInputRepresentations() const { + for (intptr_t i = 0; i < InputCount(); i++) { + Definition* input = InputAt(i)->definition(); + if (RequiredInputRepresentation(i) != input->representation()) { + return true; + } + } + + return false; +} + + void Definition::ReplaceWith(Definition* other, ForwardInstructionIterator* iterator) { // Record other's input uses. @@ -1205,7 +1217,8 @@ bool BinaryInt32OpInstr::CanDeoptimize() const { return false; case Token::kSHL: - return true; + return can_overflow() || + !RangeUtils::IsPositive(right()->definition()->range()); case Token::kMOD: { UNREACHABLE(); @@ -1228,19 +1241,14 @@ bool BinarySmiOpInstr::CanDeoptimize() const { case Token::kBIT_OR: case Token::kBIT_XOR: return false; - case Token::kSHR: { - // Can't deopt if shift-count is known positive. - Range* right_range = this->right()->definition()->range(); - return (right_range == NULL) || !right_range->IsPositive(); - } - case Token::kSHL: { - Range* right_range = this->right()->definition()->range(); - if ((right_range != NULL) && !can_overflow()) { - // Can deoptimize if right can be negative. - return !right_range->IsPositive(); - } - return true; - } + + case Token::kSHR: + return !RangeUtils::IsPositive(right()->definition()->range()); + + case Token::kSHL: + return can_overflow() || + !RangeUtils::IsPositive(right()->definition()->range()); + case Token::kMOD: { Range* right_range = this->right()->definition()->range(); return (right_range == NULL) || right_range->Overlaps(0, 0); @@ -1994,7 +2002,7 @@ Definition* BoxInt64Instr::Canonicalize(FlowGraph* flow_graph) { Definition* UnboxInstr::Canonicalize(FlowGraph* flow_graph) { - if (!HasUses()) return NULL; + if (!HasUses() && !CanDeoptimize()) return NULL; // Fold away Unbox(Box(v)). BoxInstr* box_defn = value()->definition()->AsBox(); @@ -2026,7 +2034,7 @@ Definition* UnboxInstr::Canonicalize(FlowGraph* flow_graph) { Definition* UnboxIntegerInstr::Canonicalize(FlowGraph* flow_graph) { - if (!HasUses()) return NULL; + if (!HasUses() && !CanDeoptimize()) return NULL; // Fold away UnboxInteger(BoxInteger(v)). BoxIntegerInstr* box_defn = value()->definition()->AsBoxInteger(); @@ -2040,7 +2048,7 @@ Definition* UnboxIntegerInstr::Canonicalize(FlowGraph* flow_graph) { box_defn->value()->CopyWithType(), (representation() == kUnboxedInt32) ? GetDeoptId() : Isolate::kNoDeoptId); - if ((representation() == kUnboxedInt32) && is_truncating()) { + if ((representation() == kUnboxedInt32) && !CanDeoptimize()) { converter->mark_truncating(); } flow_graph->InsertBefore(this, converter, env(), FlowGraph::kValue); @@ -2071,6 +2079,9 @@ Definition* UnboxInt32Instr::Canonicalize(FlowGraph* flow_graph) { UnboxedConstantInstr* uc = new UnboxedConstantInstr(c->value(), kUnboxedInt32); + if (c->range() != NULL) { + uc->set_range(*c->range()); + } flow_graph->InsertBefore(this, uc, NULL, FlowGraph::kValue); return uc; } @@ -2250,19 +2261,12 @@ Instruction* BranchInstr::Canonicalize(FlowGraph* flow_graph) { return this; } ComparisonInstr* comp = replacement->AsComparison(); - if ((comp == NULL) || comp->CanDeoptimize()) { + if ((comp == NULL) || + comp->CanDeoptimize() || + comp->HasUnmatchedInputRepresentations()) { return this; } - // Assert that the comparison is not serving as a pending deoptimization - // target for conversions. - for (intptr_t i = 0; i < comp->InputCount(); i++) { - if (comp->RequiredInputRepresentation(i) != - comp->InputAt(i)->definition()->representation()) { - return this; - } - } - // Replace the comparison if the replacement is used at this branch, // and has exactly one use. Value* use = comp->input_use_list(); diff --git a/runtime/vm/intermediate_language.h b/runtime/vm/intermediate_language.h index 63e4d89647f..6d8c42ebb2b 100644 --- a/runtime/vm/intermediate_language.h +++ b/runtime/vm/intermediate_language.h @@ -741,6 +741,8 @@ FOR_EACH_ABSTRACT_INSTRUCTION(INSTRUCTION_TYPE_CHECK) lifetime_position_ = pos; } + bool HasUnmatchedInputRepresentations() const; + // Returns representation expected for the input operand at the given index. virtual Representation RequiredInputRepresentation(intptr_t idx) const { return kTagged; diff --git a/tests/language/vm/canonicalization_preserves_deopt_test.dart b/tests/language/vm/canonicalization_preserves_deopt_test.dart new file mode 100644 index 00000000000..76c97a6577c --- /dev/null +++ b/tests/language/vm/canonicalization_preserves_deopt_test.dart @@ -0,0 +1,41 @@ +// Copyright (c) 2014, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +// VMOptions=--optimization_counter_threshold=10 --no-use-osr + +import "package:expect/expect.dart"; + +class X { + operator * (other) => "NaNNaNNaNNaNBatman"; +} + +foo(x) => (x * 1.0) is double; + +bar(x) { + try { + int i = (x * 1); + return true; + } catch (e) { + return false; + } +} + +baz(x) => (x * 1) == x; + +main() { + for (var i = 0; i < 100; i++) { + Expect.isTrue(foo(1.0)); + assert(() { + Expect.isTrue(bar(-1 << 63)); + return true; + }); + Expect.isTrue(baz(-1 << 63)); + } + Expect.isFalse(foo(new X())); + assert(() { + Expect.isFalse(bar(new X())); + return true; + }); + Expect.isFalse(baz(new X())); +} +