[vm] Fix a bug in the specialization of implicit setters
CallSpecializer::TryInlineInstanceSetter was using IsReceiver to determine
type check could be skipped. However it is incorrect to use IsReceiver,
because it uses SSA notion of the receiver - rather than a syntactic
notion and all strong mode guarantees apply at syntactic level.
Additionally this CL expands support for exactness tracking to PropertySet
operations and uses it to eliminate checks on setters.
Change-Id: Idce559c4ca4c88f5788d3b6b9955b5af076bd7eb
Reviewed-on: https://dart-review.googlesource.com/72442
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
diff --git a/pkg/vm/lib/transformations/call_site_annotator.dart b/pkg/vm/lib/transformations/call_site_annotator.dart
index 3d80936..c2ca324 100644
--- a/pkg/vm/lib/transformations/call_site_annotator.dart
+++ b/pkg/vm/lib/transformations/call_site_annotator.dart
@@ -55,22 +55,27 @@
}
@override
- visitMethodInvocation(MethodInvocation node) {
- super.visitMethodInvocation(node);
+ visitPropertySet(PropertySet node) {
+ super.visitPropertySet(node);
- if (shouldAnnotate(node)) {
+ if (hasGenericCovariantParameters(node.interfaceTarget)) {
_metadata.mapping[node] = new CallSiteAttributesMetadata(
receiverType: node.receiver.getStaticType(env));
}
}
- // TODO(vegorov) handle setters as well.
- // TODO(34162): We don't need to save the type here, just whether or not it's
- // a statically-checked call.
- static bool shouldAnnotate(MethodInvocation node) =>
- (node.interfaceTarget != null &&
- hasGenericCovariantParameters(node.interfaceTarget)) ||
- node.name.name == "call";
+ @override
+ visitMethodInvocation(MethodInvocation node) {
+ super.visitMethodInvocation(node);
+
+ // TODO(34162): We don't need to save the type here for calls, just whether
+ // or not it's a statically-checked call.
+ if (node.name.name == 'call' ||
+ hasGenericCovariantParameters(node.interfaceTarget)) {
+ _metadata.mapping[node] = new CallSiteAttributesMetadata(
+ receiverType: node.receiver.getStaticType(env));
+ }
+ }
/// Return [true] if the given list of [VariableDeclaration] contains
/// any annotated with generic-covariant-impl.
@@ -84,6 +89,8 @@
return containsGenericCovariantImpl(
member.function.positionalParameters) ||
containsGenericCovariantImpl(member.function.namedParameters);
+ } else if (member is Field) {
+ return member.isGenericCovariantImpl;
}
return false;
diff --git a/pkg/vm/testcases/bytecode/type_ops.dart.expect b/pkg/vm/testcases/bytecode/type_ops.dart.expect
index 924e681..457d5bc 100644
--- a/pkg/vm/testcases/bytecode/type_ops.dart.expect
+++ b/pkg/vm/testcases/bytecode/type_ops.dart.expect
@@ -175,7 +175,7 @@
if(y is self::C<dynamic, self::D::Q, core::List<self::D::P>>) {
core::print("22");
}
- this.{self::D::foo} = y as{TypeError} core::Map<self::D::P, self::D::Q>;
+ [@vm.call-site-attributes.metadata=receiverType:#lib::D<#lib::D::P, #lib::D::Q>] this.{self::D::foo} = y as{TypeError} core::Map<self::D::P, self::D::Q>;
}
[@vm.bytecode=
Bytecode {
diff --git a/runtime/vm/compiler/call_specializer.cc b/runtime/vm/compiler/call_specializer.cc
index d0a4226..a7da1b2 100644
--- a/runtime/vm/compiler/call_specializer.cc
+++ b/runtime/vm/compiler/call_specializer.cc
@@ -958,6 +958,18 @@
break;
}
+ // True if we can use unchecked entry into the setter.
+ bool is_unchecked_call = false;
+ if (!FLAG_precompiled_mode) {
+ if (unary_ic_data.NumberOfChecks() == 1 &&
+ unary_ic_data.GetExactnessAt(0).IsExact()) {
+ if (unary_ic_data.GetExactnessAt(0).IsTriviallyExact()) {
+ flow_graph()->AddExactnessGuard(instr, unary_ic_data.GetCidAt(0));
+ }
+ is_unchecked_call = true;
+ }
+ }
+
if (I->use_field_guards()) {
if (field.guarded_cid() != kDynamicCid) {
InsertBefore(instr,
@@ -1003,10 +1015,13 @@
needs_check = true;
} else if (is_generic_covariant) {
// If field is generic covariant then we don't need to check it
- // if we know that actual type arguments match static type arguments
- // e.g. if this is an invocation on this (an instance we are storing
- // into is also a receiver of a surrounding method).
- needs_check = !flow_graph_->IsReceiver(instr->ArgumentAt(0));
+ // if the invocation was marked as unchecked (e.g. receiver of
+ // the invocation is also the receiver of the surrounding method).
+ // Note: we can't use flow_graph()->IsReceiver() for this optimization
+ // because strong mode only gives static guarantees at the AST level
+ // not at the SSA level.
+ needs_check = !(is_unchecked_call ||
+ (instr->entry_kind() == Code::EntryKind::kUnchecked));
} else {
// The rest of the stores are checked statically (we are not at
// a dynamic invocation).
diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
index ad4ad84..238760e 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -3003,16 +3003,32 @@
const DirectCallMetadata direct_call =
direct_call_metadata_helper_.GetDirectTargetForPropertySet(offset);
+ const CallSiteAttributesMetadata call_site_attributes =
+ call_site_attributes_metadata_helper_.GetCallSiteAttributes(offset);
+
+ // True if callee can skip argument type checks.
+ bool is_unchecked_call = false;
+#ifndef TARGET_ARCH_DBC
+ if (call_site_attributes.receiver_type != nullptr &&
+ call_site_attributes.receiver_type->HasResolvedTypeClass() &&
+ !Class::Handle(call_site_attributes.receiver_type->type_class())
+ .IsGeneric()) {
+ is_unchecked_call = true;
+ }
+#endif
Fragment instructions(MakeTemp());
LocalVariable* variable = MakeTemporary();
const TokenPosition position = ReadPosition(); // read position.
- if (p != NULL) *p = position;
+ if (p != nullptr) *p = position;
+ if (PeekTag() == kThisExpression) {
+ is_unchecked_call = true;
+ }
instructions += BuildExpression(); // read receiver.
- LocalVariable* receiver = NULL;
+ LocalVariable* receiver = nullptr;
if (direct_call.check_receiver_for_null_) {
// Duplicate receiver for CheckNull before it is consumed by PushArgument.
receiver = MakeTemporary();
@@ -3042,10 +3058,12 @@
}
if (!direct_call.target_.IsNull()) {
+ // TODO(#34162): Pass 'is_unchecked_call' down if/when we feature multiple
+ // entry-points in AOT.
ASSERT(FLAG_precompiled_mode);
instructions +=
StaticCall(position, direct_call.target_, 2, Array::null_array(),
- ICData::kNoRebind, /* result_type = */ NULL);
+ ICData::kNoRebind, /*result_type=*/nullptr);
} else {
const intptr_t kTypeArgsLen = 0;
const intptr_t kNumArgsChecked = 1;
@@ -3057,10 +3075,12 @@
Z, Function::CreateDynamicInvocationForwarderName(setter_name));
}
- instructions +=
- InstanceCall(position, *mangled_name, Token::kSET, kTypeArgsLen, 2,
- Array::null_array(), kNumArgsChecked, *interface_target,
- /* result_type = */ NULL);
+ instructions += InstanceCall(
+ position, *mangled_name, Token::kSET, kTypeArgsLen, 2,
+ Array::null_array(), kNumArgsChecked, *interface_target,
+ /*result_type=*/nullptr,
+ /*use_unchecked_entry=*/!FLAG_precompiled_mode && is_unchecked_call,
+ &call_site_attributes);
}
instructions += Drop(); // Drop result of the setter invocation.
diff --git a/tests/language_2/this_as_covariant_call_checks_test.dart b/tests/language_2/this_as_covariant_call_checks_test.dart
new file mode 100644
index 0000000..28c79cc
--- /dev/null
+++ b/tests/language_2/this_as_covariant_call_checks_test.dart
@@ -0,0 +1,56 @@
+// 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=--no-background-compilation --enable-inlining-annotations --optimization-counter-threshold=10
+
+import "package:expect/expect.dart";
+
+const NeverInline = "NeverInline";
+
+class A<T> {
+ T field;
+
+ @NeverInline
+ set property(T v) {}
+
+ @NeverInline
+ void method(T x) {}
+
+ @NeverInline
+ void testMethod(bool violateType) {
+ A<dynamic> x = this;
+ x.method(violateType ? 10 : "10");
+ }
+
+ @NeverInline
+ void testSetter(bool violateType) {
+ A<dynamic> x = this;
+ x.property = violateType ? 10 : "10";
+ }
+
+ @NeverInline
+ void testField(bool violateType) {
+ A<dynamic> x = this;
+ x.field = violateType ? 10 : "10";
+ }
+}
+
+@NeverInline
+void loop(A<String> obj, bool violateType) {
+ for (var i = 0; i < 100; i++) {
+ obj.testMethod(violateType);
+ obj.testSetter(violateType);
+ obj.testField(violateType);
+ }
+}
+
+void main() {
+ A<num>().field = 10;
+ final obj = A<String>();
+ loop(obj, false);
+ loop(obj, false);
+ Expect.throwsTypeError(() => obj.testMethod(true));
+ Expect.throwsTypeError(() => obj.testSetter(true));
+ Expect.throwsTypeError(() => obj.testField(true));
+}
diff --git a/tests/language_2/this_as_dynamic_call_checks_test.dart b/tests/language_2/this_as_dynamic_call_checks_test.dart
index cf0cb54..dd215eb 100644
--- a/tests/language_2/this_as_dynamic_call_checks_test.dart
+++ b/tests/language_2/this_as_dynamic_call_checks_test.dart
@@ -9,23 +9,47 @@
const NeverInline = "NeverInline";
class A<T> {
- @NeverInline
- void doSet(T x) {}
+ T field;
- void doIt({bool violateType: false}) {
+ @NeverInline
+ set property(T v) {}
+
+ @NeverInline
+ void method(T x) {}
+
+ @NeverInline
+ void testMethod({bool violateType: false}) {
dynamic x = this;
- x.doSet(violateType ? 10 : "10");
+ x.method(violateType ? 10 : "10");
+ }
+
+ @NeverInline
+ void testSetter({bool violateType: false}) {
+ dynamic x = this;
+ x.property = violateType ? 10 : "10";
+ }
+
+ @NeverInline
+ void testField({bool violateType: false}) {
+ dynamic x = this;
+ x.field = violateType ? 10 : "10";
}
}
@NeverInline
void loop(A<String> obj, {bool violateType: false}) {
- for (var i = 0; i < 100; i++) obj.doIt(violateType: violateType);
+ for (var i = 0; i < 100; i++) {
+ obj.testMethod(violateType: violateType);
+ obj.testSetter(violateType: violateType);
+ obj.testField(violateType: violateType);
+ }
}
void main() {
final obj = A<String>();
loop(obj, violateType: false);
loop(obj, violateType: false);
- Expect.throwsTypeError(() => loop(obj, violateType: true));
+ Expect.throwsTypeError(() => obj.testMethod(violateType: true));
+ Expect.throwsTypeError(() => obj.testSetter(violateType: true));
+ Expect.throwsTypeError(() => obj.testField(violateType: true));
}
