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 .
2016-12-19 23:08:26 +01:00
parent 13bd1ad0bf
commit 1fcc7384aa
8 changed files with 112 additions and 53 deletions
@@ -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));
@@ -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;
 }


@@ -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:
@@ -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.
@@ -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)
 }
@@ -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();
 }

@@ -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<uword>(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();
  }

@@ -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();
+  }
+}
+
+