Revert "[vm/compiler] Move AssertAssignables out of closure bodies."

This reverts commit 42c76fd.

Reason for revert: Failures in flutter/google3. Initial hypothesis is related to product mode and instruction deduplication.

Original change's description:
> [vm/compiler] Move AssertAssignables out of closure bodies.
>
> This CL moves the final set of checks out of closure bodies and into
> dynamic closure call dispatchers.  It also adds stubs for checking top
> types and null assignability for types only known at runtime.
>
> Fixes #40813 .
>
> Changes in Flutter gallery in release mode:
>
> * arm7: -3.05% total, +0.99% vmisolate, -0.89% isolate,
>         -1.20% readonly, -4.43% instructions
> * arm8: -3.20% total, +0.99% vmisolate, -0.88% isolate,
>         -1.18% readonly, -5.05% instructions
>
> TEST=Run on trybots of all architectures, includes test adjustments where needed.
>
> Cq-Include-Trybots: luci.dart.try:vm-kernel-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-nnbd-linux-debug-ia32-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-simarm_x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-nnbd-linux-release-simarm-try,vm-kernel-nnbd-linux-release-simarm64-try,vm-kernel-precomp-linux-release-simarm-try,vm-kernel-precomp-linux-release-simarm64-try,vm-kernel-precomp-nnbd-linux-release-simarm64-try
> Change-Id: Ifb136c64339be76a642ecbb4fda26b6ce8f871f9
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166622
> Commit-Queue: Tess Strickland <[email protected]>
> Reviewed-by: Martin Kustermann <[email protected]>
> Reviewed-by: Régis Crelier <[email protected]>

[email protected],[email protected],[email protected]

Change-Id: Iaf79acddcf18fb3699a894950b31515d6f759349
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/172643
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Tess Strickland <[email protected]>

Author: sstrickl

runtime/tests/vm/dart/entrypoints/tearoff_prologue_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.
+//
+// No type checks are removed here, but we can skip the argument count check.
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10 -Denable_inlining=true
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=-1
+
+import "package:expect/expect.dart";
+import "common.dart";
+
+class C<T> {
+  @NeverInline
+  @pragma("vm:testing.unsafe.trace-entrypoints-fn", validateTearoff)
+  @pragma("vm:entry-point")
+  void samir1(T x) {
+    if (x == -1) {
+      throw "oh no";
+    }
+  }
+}
+
+void run(void Function(int) test, int i) {
+  test(i);
+}
+
+main(List<String> args) {
+  var c = new C<int>();
+  var f = c.samir1;
+
+  const int iterations = benchmarkMode ? 100000000 : 100;
+  for (int i = 0; i < iterations; ++i) {
+    run(f, i);
+  }
+
+  entryPoint.expectChecked(iterations);
+  tearoffEntryPoint.expectUnchecked(iterations);
+}

runtime/tests/vm/dart/entrypoints/tearoff_test.dart

@@ -0,0 +1,38 @@
+// Copyright (c) 2018, 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=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10 -Denable_inlining=true
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=-1
+
+// Test that typed calls against tearoffs go into the unchecked entrypoint.
+
+import "package:expect/expect.dart";
+import "common.dart";
+
+class C<T> {
+  @NeverInline
+  @pragma("vm:testing.unsafe.trace-entrypoints-fn", validateTearoff)
+  @pragma("vm:entry-point")
+  void target1(T x, String y) {
+    Expect.notEquals(x, -1);
+    Expect.equals(y, "foo");
+  }
+}
+
+void run(void Function(int, String) fn, int i) {
+  fn(i, "foo");
+}
+
+main(List<String> args) {
+  var f = (new C<int>()).target1;
+
+  const int iterations = benchmarkMode ? 100000000 : 100;
+  for (int i = 0; i < iterations; ++i) {
+    run(f, i);
+  }
+
+  entryPoint.expectChecked(iterations);
+  tearoffEntryPoint.expectUnchecked(iterations);
+}

runtime/tests/vm/dart_2/entrypoints/tearoff_prologue_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.
+//
+// No type checks are removed here, but we can skip the argument count check.
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10 -Denable_inlining=true
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=-1
+
+import "package:expect/expect.dart";
+import "common.dart";
+
+class C<T> {
+  @NeverInline
+  @pragma("vm:testing.unsafe.trace-entrypoints-fn", validateTearoff)
+  @pragma("vm:entry-point")
+  void samir1(T x) {
+    if (x == -1) {
+      throw "oh no";
+    }
+  }
+}
+
+void run(void Function(int) test, int i) {
+  test(i);
+}
+
+main(List<String> args) {
+  var c = new C<int>();
+  var f = c.samir1;
+
+  const int iterations = benchmarkMode ? 100000000 : 100;
+  for (int i = 0; i < iterations; ++i) {
+    run(f, i);
+  }
+
+  entryPoint.expectChecked(iterations);
+  tearoffEntryPoint.expectUnchecked(iterations);
+}

runtime/tests/vm/dart_2/entrypoints/tearoff_test.dart

@@ -0,0 +1,38 @@
+// Copyright (c) 2018, 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=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=10 -Denable_inlining=true
+// VMOptions=--enable-testing-pragmas --no-background-compilation --optimization-counter-threshold=-1
+
+// Test that typed calls against tearoffs go into the unchecked entrypoint.
+
+import "package:expect/expect.dart";
+import "common.dart";
+
+class C<T> {
+  @NeverInline
+  @pragma("vm:testing.unsafe.trace-entrypoints-fn", validateTearoff)
+  @pragma("vm:entry-point")
+  void target1(T x, String y) {
+    Expect.notEquals(x, -1);
+    Expect.equals(y, "foo");
+  }
+}
+
+void run(void Function(int, String) fn, int i) {
+  fn(i, "foo");
+}
+
+main(List<String> args) {
+  var f = (new C<int>()).target1;
+
+  const int iterations = benchmarkMode ? 100000000 : 100;
+  for (int i = 0; i < iterations; ++i) {
+    run(f, i);
+  }
+
+  entryPoint.expectChecked(iterations);
+  tearoffEntryPoint.expectUnchecked(iterations);
+}

runtime/vm/compiler/assembler/assembler_arm.h

@@ -1023,12 +1023,6 @@ class Assembler : public AssemblerBase {
                 JumpDistance distance = kFarJump) {
     b(label, condition);
   }
-  void BranchIfZero(Register rn,
-                    Label* label,
-                    JumpDistance distance = kFarJump) {
-    cmp(rn, Operand(0));
-    b(label, ZERO);
-  }
 
   void MoveRegister(Register rd, Register rm, Condition cond = AL);
 

runtime/vm/compiler/assembler/assembler_arm64.h

@@ -1109,11 +1109,6 @@ class Assembler : public AssemblerBase {
                 JumpDistance distance = kFarJump) {
     b(label, condition);
   }
-  void BranchIfZero(Register rn,
-                    Label* label,
-                    JumpDistance distance = kFarJump) {
-    cbz(label, rn);
-  }
 
   void cbz(Label* label, Register rt, OperandSize sz = kEightBytes) {
     EmitCompareAndBranch(CBZ, rt, label, sz);

runtime/vm/compiler/assembler/assembler_ia32.cc

@@ -1784,12 +1784,10 @@ void Assembler::LoadFromOffset(Register reg,
 }
 
 void Assembler::LoadFromStack(Register dst, intptr_t depth) {
-  ASSERT(depth >= 0);
   movl(dst, Address(ESP, depth * target::kWordSize));
 }
 
 void Assembler::StoreToStack(Register src, intptr_t depth) {
-  ASSERT(depth >= 0);
   movl(Address(ESP, depth * target::kWordSize), src);
 }
 

runtime/vm/compiler/assembler/assembler_ia32.h

@@ -577,12 +577,6 @@ class Assembler : public AssemblerBase {
                 JumpDistance distance = kFarJump) {
     j(condition, label, distance);
   }
-  void BranchIfZero(Register src,
-                    Label* label,
-                    JumpDistance distance = kFarJump) {
-    cmpl(src, Immediate(0));
-    j(ZERO, label, distance);
-  }
 
   void LoadFromOffset(Register reg,
                       Register base,
@@ -722,11 +716,6 @@ class Assembler : public AssemblerBase {
     cmpxchgl(address, reg);
   }
 
-  void CompareTypeNullabilityWith(Register type, int8_t value) {
-    cmpb(FieldAddress(type, compiler::target::Type::nullability_offset()),
-         Immediate(value));
-  }
-
   void EnterFrame(intptr_t frame_space);
   void LeaveFrame();
   void ReserveAlignedFrameSpace(intptr_t frame_space);

runtime/vm/compiler/assembler/assembler_x64.h

@@ -683,12 +683,6 @@ class Assembler : public AssemblerBase {
                 JumpDistance distance = kFarJump) {
     j(condition, label, distance);
   }
-  void BranchIfZero(Register src,
-                    Label* label,
-                    JumpDistance distance = kFarJump) {
-    cmpq(src, Immediate(0));
-    j(ZERO, label, distance);
-  }
 
   // Issues a move instruction if 'to' is not the same as 'from'.
   void MoveRegister(Register to, Register from);

runtime/vm/compiler/backend/flow_graph_compiler.cc

@@ -2286,22 +2286,15 @@ void FlowGraphCompiler::GenerateCidRangesCheck(
 
 bool FlowGraphCompiler::CheckAssertAssignableTypeTestingABILocations(
     const LocationSummary& locs) {
-  ASSERT(locs.in(AssertAssignableInstr::kInstancePos).IsRegister() &&
-         locs.in(AssertAssignableInstr::kInstancePos).reg() ==
-             TypeTestABI::kInstanceReg);
-  ASSERT((locs.in(AssertAssignableInstr::kDstTypePos).IsConstant() &&
-          locs.in(AssertAssignableInstr::kDstTypePos)
-              .constant()
-              .IsAbstractType()) ||
-         (locs.in(AssertAssignableInstr::kDstTypePos).IsRegister() &&
-          locs.in(AssertAssignableInstr::kDstTypePos).reg() ==
-              TypeTestABI::kDstTypeReg));
-  ASSERT(locs.in(AssertAssignableInstr::kInstantiatorTAVPos).IsRegister() &&
-         locs.in(AssertAssignableInstr::kInstantiatorTAVPos).reg() ==
-             TypeTestABI::kInstantiatorTypeArgumentsReg);
-  ASSERT(locs.in(AssertAssignableInstr::kFunctionTAVPos).IsRegister() &&
-         locs.in(AssertAssignableInstr::kFunctionTAVPos).reg() ==
-             TypeTestABI::kFunctionTypeArgumentsReg);
+  ASSERT(locs.in(0).IsRegister() &&
+         locs.in(0).reg() == TypeTestABI::kInstanceReg);
+  ASSERT((locs.in(1).IsConstant() && locs.in(1).constant().IsAbstractType()) ||
+         (locs.in(1).IsRegister() &&
+          locs.in(1).reg() == TypeTestABI::kDstTypeReg));
+  ASSERT(locs.in(2).IsRegister() &&
+         locs.in(2).reg() == TypeTestABI::kInstantiatorTypeArgumentsReg);
+  ASSERT(locs.in(3).IsRegister() &&
+         locs.in(3).reg() == TypeTestABI::kFunctionTypeArgumentsReg);
   ASSERT(locs.out(0).IsRegister() &&
          locs.out(0).reg() == TypeTestABI::kInstanceReg);
   return true;
@@ -2767,49 +2760,23 @@ void FlowGraphCompiler::GenerateAssertAssignable(CompileType* receiver_type,
   ASSERT(!token_pos.IsClassifying());
   ASSERT(CheckAssertAssignableTypeTestingABILocations(*locs));
 
-  // Non-null if we have a constant destination type.
-  const auto& dst_type =
-      locs->in(AssertAssignableInstr::kDstTypePos).IsConstant()
-          ? AbstractType::Cast(
-                locs->in(AssertAssignableInstr::kDstTypePos).constant())
-          : Object::null_abstract_type();
-
-  if (!dst_type.IsNull()) {
-    ASSERT(dst_type.IsFinalized());
-    if (dst_type.IsTopTypeForSubtyping()) return;  // No code needed.
+  if (!locs->in(1).IsConstant()) {
+    // TODO(dartbug.com/40813): Handle setting up the non-constant case.
+    UNREACHABLE();
   }
+  const auto& dst_type = AbstractType::Cast(locs->in(1).constant());
+  ASSERT(dst_type.IsFinalized());
+
+  if (dst_type.IsTopTypeForSubtyping()) return;  // No code needed.
 
   compiler::Label done;
-  Register type_reg = TypeTestABI::kDstTypeReg;
-  // Generate caller-side checks to perform prior to calling the TTS.
-  if (dst_type.IsNull()) {
-    __ Comment("AssertAssignable for runtime type");
-    // kDstTypeReg should already contain the destination type.
-    const bool null_safety = isolate()->null_safety();
-    GenerateStubCall(token_pos,
-                     StubCode::GetTypeIsTopTypeForSubtyping(null_safety),
-                     PcDescriptorsLayout::kOther, locs, deopt_id);
-    // TypeTestABI::kSubtypeTestCacheReg is 0 if the type is a top type.
-    __ BranchIfZero(TypeTestABI::kSubtypeTestCacheReg, &done,
-                    compiler::Assembler::kNearJump);
-
-    GenerateStubCall(token_pos,
-                     StubCode::GetNullIsAssignableToType(null_safety),
-                     PcDescriptorsLayout::kOther, locs, deopt_id);
-    // TypeTestABI::kSubtypeTestCacheReg is 0 if the object is null and is
-    // assignable.
-    __ BranchIfZero(TypeTestABI::kSubtypeTestCacheReg, &done,
-                    compiler::Assembler::kNearJump);
-  } else {
-    __ Comment("AssertAssignable for compile-time type");
-    GenerateCallerChecksForAssertAssignable(receiver_type, dst_type, &done);
-    if (dst_type.IsTypeParameter()) {
-      // The resolved type parameter is in the scratch register.
-      type_reg = TypeTestABI::kScratchReg;
-    }
-  }
 
-  GenerateTTSCall(token_pos, deopt_id, type_reg, dst_type, dst_name, locs);
+  GenerateCallerChecksForAssertAssignable(receiver_type, dst_type, &done);
+
+  GenerateTTSCall(token_pos, deopt_id,
+                  dst_type.IsTypeParameter() ? TypeTestABI::kScratchReg
+                                             : TypeTestABI::kDstTypeReg,
+                  dst_type, dst_name, locs);
   __ Bind(&done);
 }
 
@@ -2822,7 +2789,8 @@ void FlowGraphCompiler::GenerateTTSCall(TokenPosition token_pos,
                                         const AbstractType& dst_type,
                                         const String& dst_name,
                                         LocationSummary* locs) {
-  ASSERT(!dst_name.IsNull());
+  // For now, we don't allow dynamic (non-compile-time) dst_type/dst_name.
+  ASSERT(!dst_type.IsNull() && !dst_name.IsNull());
   // We use 2 consecutive entries in the pool for the subtype cache and the
   // destination name.  The second entry, namely [dst_name] seems to be unused,
   // but it will be used by the code throwing a TypeError if the type test fails
@@ -2836,11 +2804,9 @@ void FlowGraphCompiler::GenerateTTSCall(TokenPosition token_pos,
   ASSERT((sub_type_cache_index + 1) == dst_name_index);
   ASSERT(__ constant_pool_allowed());
 
-  __ Comment("TTSCall");
   // If the dst_type is known at compile time and instantiated, we know the
   // target TTS stub and so can use a PC-relative call when available.
-  if (!dst_type.IsNull() && dst_type.IsInstantiated() &&
-      CanPcRelativeCall(dst_type)) {
+  if (dst_type.IsInstantiated() && CanPcRelativeCall(dst_type)) {
     __ LoadWordFromPoolIndex(TypeTestABI::kSubtypeTestCacheReg,
                              sub_type_cache_index);
     __ GenerateUnRelocatedPcRelativeCall();