From 1fcc7384aa28aa2ca17b8ec304dd2c4e083c36cd Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Mon, 19 Dec 2016 23:08:26 +0100 Subject: [PATCH] VM: [DBC] Fix lazy deoptimization after calls that return no values. On DBC calls return value on the stack instead of a dedicate register but not all calls have a return value (e.g. CheckStack does not) and such calls don't push anything. Implementation of a lazy deopt however assumed that value is always present and tried to manually preserve it by popping and pushing it back after the frame was rewritten. This of course damaged the frame if we performed a lazy deoptimization after the call that did not push anything. Instead of manually preserving result value value use frame translation to handle it. This allows to handle calls that return value and those that do not uniformly in the Deopt bytecode. Compiler takes care of creating the right deoptimization environment instead. BUG= R=zra@google.com Review-Url: https://codereview.chromium.org/2587133002 . --- runtime/vm/deopt_instructions.cc | 8 ++++- runtime/vm/flow_graph_compiler.cc | 3 +- runtime/vm/flow_graph_compiler.h | 27 +++++++++++++-- runtime/vm/flow_graph_compiler_dbc.cc | 19 ++++++++--- runtime/vm/intermediate_language.cc | 6 ++-- runtime/vm/intermediate_language_dbc.cc | 22 ++++++------ runtime/vm/simulator_dbc.cc | 41 ++++++----------------- tests/language/vm/lazy_deopt_vm_test.dart | 39 +++++++++++++++++++++ 8 files changed, 112 insertions(+), 53 deletions(-) create mode 100644 tests/language/vm/lazy_deopt_vm_test.dart diff --git a/runtime/vm/deopt_instructions.cc b/runtime/vm/deopt_instructions.cc index 31f08cd3924..345db49623f 100644 --- a/runtime/vm/deopt_instructions.cc +++ b/runtime/vm/deopt_instructions.cc @@ -1077,7 +1077,13 @@ void DeoptInfoBuilder::AddCopy(Value* value, deopt_instr = new (zone()) DeoptMaterializedObjectRefInstr(index); } else { ASSERT(!source_loc.IsInvalid()); - switch (value->definition()->representation()) { +#if defined(TARGET_ARCH_DBC) + Representation rep = + (value == NULL) ? kTagged : value->definition()->representation(); +#else + Representation rep = value->definition()->representation(); +#endif + switch (rep) { case kTagged: deopt_instr = new (zone()) DeoptWordInstr(ToCpuRegisterSource(source_loc)); diff --git a/runtime/vm/flow_graph_compiler.cc b/runtime/vm/flow_graph_compiler.cc index a3db43828ea..d0a2cb111a7 100644 --- a/runtime/vm/flow_graph_compiler.cc +++ b/runtime/vm/flow_graph_compiler.cc @@ -791,7 +791,7 @@ void FlowGraphCompiler::AddStubCallTarget(const Code& code) { } -void FlowGraphCompiler::AddDeoptIndexAtCall(intptr_t deopt_id) { +CompilerDeoptInfo* FlowGraphCompiler::AddDeoptIndexAtCall(intptr_t deopt_id) { ASSERT(is_optimizing()); ASSERT(!intrinsic_mode()); CompilerDeoptInfo* info = @@ -800,6 +800,7 @@ void FlowGraphCompiler::AddDeoptIndexAtCall(intptr_t deopt_id) { pending_deoptimization_env_); info->set_pc_offset(assembler()->CodeSize()); deopt_infos_.Add(info); + return info; } diff --git a/runtime/vm/flow_graph_compiler.h b/runtime/vm/flow_graph_compiler.h index e8b013fdc58..8d8606b8704 100644 --- a/runtime/vm/flow_graph_compiler.h +++ b/runtime/vm/flow_graph_compiler.h @@ -122,6 +122,9 @@ class CompilerDeoptInfo : public ZoneAllocated { deopt_id_(deopt_id), reason_(reason), flags_(flags), +#if defined(TARGET_ARCH_DBC) + lazy_deopt_with_result_(false), +#endif deopt_env_(deopt_env) { ASSERT(deopt_env != NULL); } @@ -143,6 +146,18 @@ class CompilerDeoptInfo : public ZoneAllocated { uint32_t flags() const { return flags_; } const Environment* deopt_env() const { return deopt_env_; } +#if defined(TARGET_ARCH_DBC) + // On DBC calls return results on the stack but not all calls have a result. + // This needs to be taken into account when constructing lazy deoptimization + // environment. + // For calls with results we add a deopt instruction that would copy top + // of the stack from optimized frame to unoptimized frame effectively + // preserving the result of the call. + // For calls with no results we don't emit such instruction - because there + // is no result pushed by the return sequence. + void mark_lazy_deopt_with_result() { lazy_deopt_with_result_ = true; } +#endif + private: void EmitMaterializations(Environment* env, DeoptInfoBuilder* builder); @@ -153,6 +168,9 @@ class CompilerDeoptInfo : public ZoneAllocated { const intptr_t deopt_id_; const ICData::DeoptReasonId reason_; const uint32_t flags_; +#if defined(TARGET_ARCH_DBC) + bool lazy_deopt_with_result_; +#endif Environment* deopt_env_; DISALLOW_COPY_AND_ASSIGN(CompilerDeoptInfo); @@ -499,7 +517,7 @@ class FlowGraphCompiler : public ValueObject { uint16_t ToEmbeddableCid(intptr_t cid, Instruction* instruction); #endif // defined(TARGET_ARCH_DBC) - void AddDeoptIndexAtCall(intptr_t deopt_id); + CompilerDeoptInfo* AddDeoptIndexAtCall(intptr_t deopt_id); void AddSlowPathCode(SlowPathCode* slow_path); @@ -583,11 +601,16 @@ class FlowGraphCompiler : public ValueObject { bool EndCodeSourceRange(TokenPosition token_pos); #if defined(TARGET_ARCH_DBC) + enum CallResult { + kHasResult, + kNoResult, + }; void RecordAfterCallHelper(TokenPosition token_pos, intptr_t deopt_id, intptr_t argument_count, + CallResult result, LocationSummary* locs); - void RecordAfterCall(Instruction* instr); + void RecordAfterCall(Instruction* instr, CallResult result); #endif private: diff --git a/runtime/vm/flow_graph_compiler_dbc.cc b/runtime/vm/flow_graph_compiler_dbc.cc index 745fcf98e28..eb3660a4f7d 100644 --- a/runtime/vm/flow_graph_compiler_dbc.cc +++ b/runtime/vm/flow_graph_compiler_dbc.cc @@ -118,6 +118,11 @@ RawTypedData* CompilerDeoptInfo::CreateDeoptInfo(FlowGraphCompiler* compiler, // will be able to find them during materialization. slot_ix = builder->EmitMaterializationArguments(slot_ix); + if (lazy_deopt_with_result_) { + ASSERT(reason() == ICData::kDeoptAtCall); + builder->AddCopy(NULL, Location::StackSlot(stack_height), slot_ix++); + } + // For the innermost environment, set outgoing arguments and the locals. for (intptr_t i = current->Length() - 1; i >= current->fixed_parameter_count(); i--) { @@ -179,6 +184,7 @@ RawTypedData* CompilerDeoptInfo::CreateDeoptInfo(FlowGraphCompiler* compiler, void FlowGraphCompiler::RecordAfterCallHelper(TokenPosition token_pos, intptr_t deopt_id, intptr_t argument_count, + CallResult result, LocationSummary* locs) { RecordSafepoint(locs); // Marks either the continuation point in unoptimized code or the @@ -190,7 +196,10 @@ void FlowGraphCompiler::RecordAfterCallHelper(TokenPosition token_pos, // On all other architectures caller drops outgoing arguments itself // hence the difference. pending_deoptimization_env_->DropArguments(argument_count); - AddDeoptIndexAtCall(deopt_id_after); + CompilerDeoptInfo* info = AddDeoptIndexAtCall(deopt_id_after); + if (result == kHasResult) { + info->mark_lazy_deopt_with_result(); + } // This descriptor is needed for exception handling in optimized code. AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id_after, token_pos); } else { @@ -201,9 +210,9 @@ void FlowGraphCompiler::RecordAfterCallHelper(TokenPosition token_pos, } -void FlowGraphCompiler::RecordAfterCall(Instruction* instr) { +void FlowGraphCompiler::RecordAfterCall(Instruction* instr, CallResult result) { RecordAfterCallHelper(instr->token_pos(), instr->deopt_id(), - instr->ArgumentCount(), instr->locs()); + instr->ArgumentCount(), result, instr->locs()); } @@ -263,7 +272,9 @@ void FlowGraphCompiler::GenerateAssertAssignable(TokenPosition token_pos, locs->SetStackBit(locs->out(0).reg()); } AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id, token_pos); - RecordAfterCallHelper(token_pos, deopt_id, 0, locs); + const intptr_t kArgCount = 0; + RecordAfterCallHelper(token_pos, deopt_id, kArgCount, + FlowGraphCompiler::kHasResult, locs); if (is_optimizing()) { // Assert assignable keeps the instance on the stack as the result, // all other arguments are popped. diff --git a/runtime/vm/intermediate_language.cc b/runtime/vm/intermediate_language.cc index 373aa483729..78675d997f1 100644 --- a/runtime/vm/intermediate_language.cc +++ b/runtime/vm/intermediate_language.cc @@ -3125,7 +3125,7 @@ void InstanceCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) { } compiler->AddCurrentDescriptor(RawPcDescriptors::kIcCall, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); if (compiler->is_optimizing()) { __ PopLocal(locs()->out(0).reg()); @@ -3273,7 +3273,7 @@ void StaticCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ StaticCall(ArgumentCount(), argdesc_kidx); compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); __ PopLocal(locs()->out(0).reg()); } else { const intptr_t ic_data_kidx = __ AddConstant(*call_ic_data); @@ -3281,7 +3281,7 @@ void StaticCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ IndirectStaticCall(ArgumentCount(), argdesc_kidx); compiler->AddCurrentDescriptor(RawPcDescriptors::kUnoptStaticCall, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); } #endif // !defined(TARGET_ARCH_DBC) } diff --git a/runtime/vm/intermediate_language_dbc.cc b/runtime/vm/intermediate_language_dbc.cc index efe48b14acc..60e9e940843 100644 --- a/runtime/vm/intermediate_language_dbc.cc +++ b/runtime/vm/intermediate_language_dbc.cc @@ -204,7 +204,7 @@ EMIT_NATIVE_CODE(InstanceOf, __ InstanceOf(negate_result() ? 1 : 0); compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); if (compiler->is_optimizing()) { __ PopLocal(locs()->out(0).reg()); } @@ -227,7 +227,7 @@ EMIT_NATIVE_CODE(AssertBoolean, __ AssertBoolean(Isolate::Current()->type_checks() ? 1 : 0); compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); if (compiler->is_optimizing()) { __ Drop1(); } @@ -280,7 +280,7 @@ EMIT_NATIVE_CODE(PolymorphicInstanceCall, __ StaticCall(instance_call()->ArgumentCount(), argdesc_kidx); compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), instance_call()->token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); __ PopLocal(locs()->out(0).reg()); } @@ -301,7 +301,7 @@ EMIT_NATIVE_CODE(CheckStackOverflow, } compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kNoResult); } @@ -431,7 +431,7 @@ EMIT_NATIVE_CODE(InitStaticField, } else { __ InitStaticTOS(); } - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kNoResult); } @@ -449,7 +449,7 @@ EMIT_NATIVE_CODE(ClosureCall, const intptr_t argdesc_kidx = compiler->assembler()->AddConstant(arguments_descriptor); __ StaticCall(argument_count, argdesc_kidx); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); if (compiler->is_optimizing()) { __ PopLocal(locs()->out(0).reg()); } @@ -699,11 +699,11 @@ EMIT_NATIVE_CODE(CreateArray, __ Push(type_arguments); __ Push(length); __ CreateArrayTOS(); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); __ PopLocal(out); } else { __ CreateArrayTOS(); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); } } @@ -917,7 +917,7 @@ EMIT_NATIVE_CODE(StringInterpolate, __ PushConstant(CallFunction()); const intptr_t argdesc_kidx = __ AddConstant(arguments_descriptor); __ StaticCall(kArgumentCount, argdesc_kidx); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kHasResult); if (compiler->is_optimizing()) { __ PopLocal(locs()->out(0).reg()); } @@ -1176,7 +1176,7 @@ EMIT_NATIVE_CODE(Throw, 0, Location::NoLocation(), LocationSummary::kCall) { __ Throw(0); compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kNoResult); __ Trap(); } @@ -1186,7 +1186,7 @@ EMIT_NATIVE_CODE(ReThrow, 0, Location::NoLocation(), LocationSummary::kCall) { __ Throw(1); compiler->AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id(), token_pos()); - compiler->RecordAfterCall(this); + compiler->RecordAfterCall(this, FlowGraphCompiler::kNoResult); __ Trap(); } diff --git a/runtime/vm/simulator_dbc.cc b/runtime/vm/simulator_dbc.cc index dea082ab612..b114dc6c83c 100644 --- a/runtime/vm/simulator_dbc.cc +++ b/runtime/vm/simulator_dbc.cc @@ -3554,19 +3554,13 @@ RawObject* Simulator::Call(const Code& code, { BYTECODE(Deopt, A_D); + + // Note: frame translation will take care of preserving result at the + // top of the stack. See CompilerDeoptInfo::CreateDeoptInfo. const bool is_lazy = rD == 0; - // Preserve result of the previous call. - // TODO(vegorov) we could have actually included result into the - // deoptimization environment because it is passed through the stack. - // If we do then we could remove special result handling from this code. - RawObject* result = SP[0]; - - // When not preserving the result, we still need to preserve SP[0] as it - // contains some temporary expression. - if (!is_lazy) { - SP++; - } + // Make sure we preserve SP[0] when entering synthetic frame below. + SP++; // Leaf runtime function DeoptimizeCopyFrame expects a Dart frame. // The code in this frame may not cause GC. @@ -3582,9 +3576,6 @@ RawObject* Simulator::Call(const Code& code, // We are now inside a valid frame. { - if (is_lazy) { - *++SP = result; // Preserve result (call below can cause GC). - } *++SP = 0; // Space for the result: number of materialization args. Exit(thread, FP, SP + 1, /*pc=*/0); NativeArguments native_args(thread, 0, SP, SP); @@ -3592,10 +3583,6 @@ RawObject* Simulator::Call(const Code& code, } const intptr_t materialization_arg_count = Smi::Value(RAW_CAST(Smi, *SP--)) / kWordSize; - if (is_lazy) { - // Reload the result. It might have been relocated by GC. - result = *SP--; - } // Restore caller PC. pc = SavedCallerPC(FP); @@ -3604,21 +3591,13 @@ RawObject* Simulator::Call(const Code& code, // Check if it is a fake PC marking the entry frame. ASSERT((reinterpret_cast(pc) & 2) == 0); - // Restore SP, FP and PP. Push result and dispatch. - // Note: unlike in a normal return sequence we don't need to drop - // arguments - those are not part of the innermost deoptimization - // environment they were dropped by FlowGraphCompiler::RecordAfterCall. - - // If the result is not preserved, the unoptimized frame ends at the - // next slot. - SP = FrameArguments(FP, materialization_arg_count); + // Restore SP, FP and PP. + // Unoptimized frame SP is one below FrameArguments(...) because + // FrameArguments(...) returns a pointer to the first argument. + SP = FrameArguments(FP, materialization_arg_count) - 1; FP = SavedCallerFP(FP); pp = SimulatorHelpers::FrameCode(FP)->ptr()->object_pool_->ptr(); - if (is_lazy) { - SP[0] = result; // Put the result on the stack. - } else { - SP--; // No result to push. - } + DISPATCH(); } diff --git a/tests/language/vm/lazy_deopt_vm_test.dart b/tests/language/vm/lazy_deopt_vm_test.dart new file mode 100644 index 00000000000..3f4b3923459 --- /dev/null +++ b/tests/language/vm/lazy_deopt_vm_test.dart @@ -0,0 +1,39 @@ +// Copyright (c) 2016, 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. +// Test deoptimization on an optimistically hoisted smi check. +// VMOptions=--deoptimize_every=10 --optimization-counter-threshold=10 --no-background-compilation --enable-inlining-annotations + +// Test that lazy deoptimization on stack checks does not damage unoptimized +// frame. + +import 'package:expect/expect.dart'; + + +foo() { + var a = 0; + var b = 1; + var c = 2; + var d = 3; + var e = 4; + for (var i = 0; i < 10; i++) { + a++; + b++; + c++; + d++; + e++; + } + Expect.equals(10, a); + Expect.equals(11, b); + Expect.equals(12, c); + Expect.equals(13, d); + Expect.equals(14, e); +} + +main() { + for (var i = 0; i < 10; ++i) { + foo(); + } +} + +