From 8c7d47cd19ec52da56ce9b1d92bc6995c63313b7 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Thu, 9 Mar 2023 21:18:49 +0000 Subject: [PATCH] [vm/compiler] Keep and propagate static types during local type propagation Types of record field accesses are based on static record types, so it is useful to keep and propagate static types even when concrete class id is known. TEST=runtime/tests/vm/dart/records_field_operations_il_test.dart Issue: https://github.com/dart-lang/sdk/issues/49719 Issue: https://github.com/dart-lang/sdk/issues/51637 Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-nnbd-linux-release-x64-try,vm-kernel-precomp-linux-release-x64-try Change-Id: I268e3d519b07e12d1e2f8929cbd704a6995e2053 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/287222 Reviewed-by: Martin Kustermann Commit-Queue: Alexander Markov --- pkg/vm/tool/compare_il | 2 +- .../records_field_operations_il_test.dart | 259 ++++++++++++++++++ runtime/vm/compiler/backend/slot.cc | 2 +- .../vm/compiler/backend/type_propagator.cc | 117 ++++---- 4 files changed, 328 insertions(+), 52 deletions(-) create mode 100644 runtime/tests/vm/dart/records_field_operations_il_test.dart diff --git a/pkg/vm/tool/compare_il b/pkg/vm/tool/compare_il index 4ff394d2f0e..5db629b9052 100755 --- a/pkg/vm/tool/compare_il +++ b/pkg/vm/tool/compare_il @@ -28,4 +28,4 @@ SDK_DIR="$CUR_DIR/../../.." # checked-in dart binaries must be adjusted. DART="$SDK_DIR/tools/sdks/dart-sdk/bin/dart" -exec "$DART" $DART_VM_FLAGS --enable-experiment=records "${SDK_DIR}/pkg/vm/bin/compare_il.dart" $@ +exec "$DART" $DART_VM_FLAGS --enable-experiment=records,patterns "${SDK_DIR}/pkg/vm/bin/compare_il.dart" $@ diff --git a/runtime/tests/vm/dart/records_field_operations_il_test.dart b/runtime/tests/vm/dart/records_field_operations_il_test.dart new file mode 100644 index 00000000000..688753d5cf5 --- /dev/null +++ b/runtime/tests/vm/dart/records_field_operations_il_test.dart @@ -0,0 +1,259 @@ +// Copyright (c) 2023, 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. + +// Verifies that compiler can propagate static types through record fields +// and recognize int/double operations on record fields. + +// SharedOptions=--enable-experiment=records,patterns + +import 'package:vm/testing/il_matchers.dart'; + +double d(int x) => x + double.parse('1.0'); + +(double, double) staticFieldD = (d(1), d(2)); + +abstract class A { + (double, double) instanceFieldD = (d(1), d(2)); + (double, double) instanceCallD(); +} + +class A1 extends A { + @pragma('vm:never-inline') + (double, double) instanceCallD() => (d(1), d(2)); +} + +class A2 extends A { + @pragma('vm:never-inline') + (double, double) instanceCallD() => (d(3), d(4)); +} + +@pragma('vm:never-inline') +(double, double) staticCallD() => (d(1), d(2)); + +@pragma('vm:never-inline') +@pragma('vm:testing:print-flow-graph') +void testDouble(A obj, double a, double b, (double, double) param) { + { + var local = (a, b); + var (x, y) = local; + print(x + y); + } + + { + var (x, y) = param; + print(x + y); + } + + { + var (x, y) = staticFieldD; + print(x + y); + } + + { + var (x, y) = staticCallD(); + print(x + y); + } + + { + var (x, y) = obj.instanceFieldD; + print(x + y); + } + + { + var (x, y) = obj.instanceCallD(); + print(x + y); + } +} + +void matchIL$testDouble(FlowGraph graph) { + graph.match([ + match.block('Graph'), + match.block('Function', [ + 'obj' << match.Parameter(index: 0), + 'a' << match.Parameter(index: 1), + 'b' << match.Parameter(index: 2), + 'param' << match.Parameter(index: 3), + match.CheckStackOverflow(), + 'v1' << match.BinaryDoubleOp('a', 'b'), + 'v1_boxed' << match.Box('v1'), + match.MoveArgument('v1_boxed'), + match.StaticCall(), + 'x2' << match.LoadField('param'), + 'y2' << match.LoadField('param'), + 'x2_unboxed' << match.Unbox('x2'), + 'y2_unboxed' << match.Unbox('y2'), + 'v2' << match.BinaryDoubleOp('x2_unboxed', 'y2_unboxed'), + 'v2_boxed' << match.Box('v2'), + match.MoveArgument('v2_boxed'), + match.StaticCall(), + 'rec3' << match.LoadStaticField(), + 'x3' << match.LoadField('rec3'), + 'y3' << match.LoadField('rec3'), + 'x3_unboxed' << match.Unbox('x3'), + 'y3_unboxed' << match.Unbox('y3'), + 'v3' << match.BinaryDoubleOp('x3_unboxed', 'y3_unboxed'), + 'v3_boxed' << match.Box('v3'), + match.MoveArgument('v3_boxed'), + match.StaticCall(), + 'rec4' << match.StaticCall(), + 'x4' << match.ExtractNthOutput('rec4', index: 0), + 'y4' << match.ExtractNthOutput('rec4', index: 1), + 'x4_unboxed' << match.Unbox('x4'), + 'y4_unboxed' << match.Unbox('y4'), + 'v4' << match.BinaryDoubleOp('x4_unboxed', 'y4_unboxed'), + 'v4_boxed' << match.Box('v4'), + match.MoveArgument('v4_boxed'), + match.StaticCall(), + 'rec5' << match.LoadField('obj'), + 'x5' << match.LoadField('rec5'), + 'y5' << match.LoadField('rec5'), + 'x5_unboxed' << match.Unbox('x5'), + 'y5_unboxed' << match.Unbox('y5'), + 'v5' << match.BinaryDoubleOp('x5_unboxed', 'y5_unboxed'), + 'v5_boxed' << match.Box('v5'), + match.MoveArgument('v5_boxed'), + match.StaticCall(), + 'obj_cid' << match.LoadClassId('obj'), + match.MoveArgument('obj'), + 'rec6' << match.DispatchTableCall('obj_cid'), + 'x6' << match.ExtractNthOutput('rec6', index: 0), + 'y6' << match.ExtractNthOutput('rec6', index: 1), + 'x6_unboxed' << match.Unbox('x6'), + 'y6_unboxed' << match.Unbox('y6'), + 'v6' << match.BinaryDoubleOp('x6_unboxed', 'y6_unboxed'), + 'v6_boxed' << match.Box('v6'), + match.MoveArgument('v6_boxed'), + match.Return(), + ]), + ]); +} + +int i(int x) => x + int.parse('1'); + +(int, int) staticFieldI = (i(1), i(2)); + +abstract class B { + (int, int) instanceFieldI = (i(1), i(2)); + (int, int) instanceCallI(); +} + +class B1 extends B { + @pragma('vm:never-inline') + (int, int) instanceCallI() => (i(1), i(2)); +} + +class B2 extends B { + @pragma('vm:never-inline') + (int, int) instanceCallI() => (i(3), i(4)); +} + +@pragma('vm:never-inline') +(int, int) staticCallI() => (i(1), i(2)); + +@pragma('vm:never-inline') +@pragma('vm:testing:print-flow-graph') +void testInt(B obj, int a, int b, (int, int) param) { + { + var local = (a, b); + var (x, y) = local; + print(x + y); + } + + { + var (x, y) = param; + print(x + y); + } + + { + var (x, y) = staticFieldI; + print(x + y); + } + + { + var (x, y) = staticCallI(); + print(x + y); + } + + { + var (x, y) = obj.instanceFieldI; + print(x + y); + } + + { + var (x, y) = obj.instanceCallI(); + print(x + y); + } +} + +void matchIL$testInt(FlowGraph graph) { + graph.match([ + match.block('Graph'), + match.block('Function', [ + 'obj' << match.Parameter(index: 0), + 'a' << match.Parameter(index: 1), + 'b' << match.Parameter(index: 2), + 'param' << match.Parameter(index: 3), + match.CheckStackOverflow(), + 'v1' << match.BinaryInt64Op('a', 'b'), + 'v1_boxed' << match.BoxInt64('v1'), + match.MoveArgument('v1_boxed'), + match.StaticCall(), + 'x2' << match.LoadField('param'), + 'y2' << match.LoadField('param'), + 'x2_unboxed' << match.UnboxInt64('x2'), + 'y2_unboxed' << match.UnboxInt64('y2'), + 'v2' << match.BinaryInt64Op('x2_unboxed', 'y2_unboxed'), + 'v2_boxed' << match.BoxInt64('v2'), + match.MoveArgument('v2_boxed'), + match.StaticCall(), + 'rec3' << match.LoadStaticField(), + 'x3' << match.LoadField('rec3'), + 'y3' << match.LoadField('rec3'), + 'x3_unboxed' << match.UnboxInt64('x3'), + 'y3_unboxed' << match.UnboxInt64('y3'), + 'v3' << match.BinaryInt64Op('x3_unboxed', 'y3_unboxed'), + 'v3_boxed' << match.BoxInt64('v3'), + match.MoveArgument('v3_boxed'), + match.StaticCall(), + 'rec4' << match.StaticCall(), + 'x4' << match.ExtractNthOutput('rec4', index: 0), + 'y4' << match.ExtractNthOutput('rec4', index: 1), + 'x4_unboxed' << match.UnboxInt64('x4'), + 'y4_unboxed' << match.UnboxInt64('y4'), + 'v4' << match.BinaryInt64Op('x4_unboxed', 'y4_unboxed'), + 'v4_boxed' << match.BoxInt64('v4'), + match.MoveArgument('v4_boxed'), + match.StaticCall(), + 'rec5' << match.LoadField('obj'), + 'x5' << match.LoadField('rec5'), + 'y5' << match.LoadField('rec5'), + 'x5_unboxed' << match.UnboxInt64('x5'), + 'y5_unboxed' << match.UnboxInt64('y5'), + 'v5' << match.BinaryInt64Op('x5_unboxed', 'y5_unboxed'), + 'v5_boxed' << match.BoxInt64('v5'), + match.MoveArgument('v5_boxed'), + match.StaticCall(), + 'obj_cid' << match.LoadClassId('obj'), + match.MoveArgument('obj'), + 'rec6' << match.DispatchTableCall('obj_cid'), + 'x6' << match.ExtractNthOutput('rec6', index: 0), + 'y6' << match.ExtractNthOutput('rec6', index: 1), + 'x6_unboxed' << match.UnboxInt64('x6'), + 'y6_unboxed' << match.UnboxInt64('y6'), + 'v6' << match.BinaryInt64Op('x6_unboxed', 'y6_unboxed'), + 'v6_boxed' << match.BoxInt64('v6'), + match.MoveArgument('v6_boxed'), + match.StaticCall(), + match.Return(), + ]), + ]); +} + +void main(List args) { + // Make sure all parameters are non-constant. + // Also make sure A/B is polymorphic to prevent + // devirtualization of instance calls. + testDouble(args.length > 0 ? A1() : A2(), d(1), d(2), (d(3), d(4))); + testInt(args.length > 0 ? B1() : B2(), i(1), i(2), (i(3), i(4))); +} diff --git a/runtime/vm/compiler/backend/slot.cc b/runtime/vm/compiler/backend/slot.cc index f7e4354eefa..8cbdae02ab2 100644 --- a/runtime/vm/compiler/backend/slot.cc +++ b/runtime/vm/compiler/backend/slot.cc @@ -485,7 +485,7 @@ CompileType Slot::ComputeCompileType() const { } return CompileType(is_nullable(), is_sentinel_visible(), nullable_cid(), - nullable_cid() == kDynamicCid ? static_type_ : nullptr); + static_type_); } const AbstractType& Slot::static_type() const { diff --git a/runtime/vm/compiler/backend/type_propagator.cc b/runtime/vm/compiler/backend/type_propagator.cc index a08509f28f3..55e32c008bd 100644 --- a/runtime/vm/compiler/backend/type_propagator.cc +++ b/runtime/vm/compiler/backend/type_propagator.cc @@ -1246,16 +1246,23 @@ CompileType ParameterInstr::ComputeType() const { ? pf.ParameterVariable(param_index) : pf.RawParameterVariable(param_index); ASSERT(param != nullptr); - CompileType* inferred_type = NULL; + + CompileType* inferred_type = nullptr; + intptr_t inferred_cid = kDynamicCid; + bool inferred_nullable = true; if (!block_->IsCatchBlockEntry()) { inferred_type = param->parameter_type(); - } - // Best bet: use inferred type if it is a concrete class or int. - if ((inferred_type != nullptr) && - ((inferred_type->ToNullableCid() != kDynamicCid) || - inferred_type->IsNullableInt())) { - TraceStrongModeType(this, inferred_type); - return *inferred_type; + + if (inferred_type != nullptr) { + // Use inferred type if it is an int. + if (inferred_type->IsNullableInt()) { + TraceStrongModeType(this, inferred_type); + return *inferred_type; + } + // Otherwise use inferred cid and nullability. + inferred_cid = inferred_type->ToNullableCid(); + inferred_nullable = inferred_type->is_nullable(); + } } // If parameter type was checked by caller, then use Dart type annotation, // plus non-nullability from inferred type if known. @@ -1266,15 +1273,17 @@ CompileType ParameterInstr::ComputeType() const { (param->was_type_checked_by_caller() || (is_unchecked_entry_param && !param->is_explicit_covariant_parameter()))) { - const bool is_nullable = - (inferred_type == NULL) || inferred_type->is_nullable(); - TraceStrongModeType(this, param->type()); - return CompileType::FromAbstractType( - param->type(), is_nullable, - block_->IsCatchBlockEntry() && param->is_late()); + const AbstractType& static_type = param->type(); + CompileType result( + inferred_nullable && !static_type.IsStrictlyNonNullable(), + block_->IsCatchBlockEntry() && param->is_late(), + inferred_cid == kDynamicCid ? kIllegalCid : inferred_cid, + &static_type); + TraceStrongModeType(this, &result); + return result; } - // Last resort: use inferred non-nullability. - if (inferred_type != NULL) { + // Last resort: use inferred type as is. + if (inferred_type != nullptr) { TraceStrongModeType(this, inferred_type); return *inferred_type; } @@ -1410,11 +1419,15 @@ CompileType InstanceCallBaseInstr::ComputeType() const { // TODO(alexmarkov): calculate type of InstanceCallInstr eagerly // (in optimized mode) and avoid keeping separate result_type. CompileType* inferred_type = result_type(); - if ((inferred_type != nullptr) && - ((inferred_type->ToNullableCid() != kDynamicCid) || - (!inferred_type->ToAbstractType()->IsDynamicType()))) { - TraceStrongModeType(this, inferred_type); - return *inferred_type; + intptr_t inferred_cid = kDynamicCid; + bool is_nullable = CompileType::kCanBeNull; + if (inferred_type != nullptr) { + if (inferred_type->IsNullableInt()) { + TraceStrongModeType(this, inferred_type); + return *inferred_type; + } + inferred_cid = inferred_type->ToNullableCid(); + is_nullable = inferred_type->is_nullable(); } // Include special cases of type inference for int operations. @@ -1450,16 +1463,18 @@ CompileType InstanceCallBaseInstr::ComputeType() const { // 1. receiver type inferred by the front-end is not passed to VM. // 2. VM collects type arguments through the chain of superclasses but // not through implemented interfaces. - // So treat non-instantiated generic types as dynamic to avoid pretending - // the type is known. // TODO(dartbug.com/30480): instantiate generic result_type - if (result_type.IsInstantiated()) { - TraceStrongModeType(this, result_type); - const bool is_nullable = - (inferred_type == NULL) || inferred_type->is_nullable(); - return CompileType::FromAbstractType(result_type, is_nullable, - CompileType::kCannotBeSentinel); - } + CompileType result(is_nullable && !result_type.IsStrictlyNonNullable(), + CompileType::kCannotBeSentinel, + inferred_cid == kDynamicCid ? kIllegalCid : inferred_cid, + &result_type); + TraceStrongModeType(this, &result); + return result; + } + + if (inferred_type != nullptr) { + TraceStrongModeType(this, inferred_type); + return *inferred_type; } return CompileType::Dynamic(); @@ -1529,14 +1544,17 @@ CompileType StaticCallInstr::ComputeType() const { return ComputeListFactoryType(inferred_type, ArgumentValueAt(0)); } - if ((inferred_type != nullptr) && - ((inferred_type->ToNullableCid() != kDynamicCid) || - (!inferred_type->ToAbstractType()->IsDynamicType()))) { - TraceStrongModeType(this, inferred_type); - return *inferred_type; + intptr_t inferred_cid = kDynamicCid; + bool is_nullable = CompileType::kCanBeNull; + if (inferred_type != nullptr) { + if (inferred_type->IsNullableInt()) { + TraceStrongModeType(this, inferred_type); + return *inferred_type; + } + inferred_cid = inferred_type->ToNullableCid(); + is_nullable = inferred_type->is_nullable(); } - bool is_nullable = CompileType::kCanBeNull; if (function_.has_pragma()) { const intptr_t cid = MethodRecognizer::ResultCidFromPragma(function_); if (cid != kDynamicCid) { @@ -1549,18 +1567,12 @@ CompileType StaticCallInstr::ComputeType() const { const AbstractType& result_type = AbstractType::ZoneHandle(function().result_type()); - // TODO(dartbug.com/30480): instantiate generic result_type if possible. - // Also, consider fixing AbstractType::IsSubtypeOf to handle - // non-instantiated types properly. - if (result_type.IsInstantiated()) { - TraceStrongModeType(this, result_type); - is_nullable = is_nullable && - (inferred_type == nullptr || inferred_type->is_nullable()); - return CompileType::FromAbstractType(result_type, is_nullable, - CompileType::kCannotBeSentinel); - } - - return CompileType::Dynamic(); + CompileType result(is_nullable && !result_type.IsStrictlyNonNullable(), + CompileType::kCannotBeSentinel, + inferred_cid == kDynamicCid ? kIllegalCid : inferred_cid, + &result_type); + TraceStrongModeType(this, &result); + return result; } CompileType LoadLocalInstr::ComputeType() const { @@ -1667,10 +1679,15 @@ CompileType LoadClassIdInstr::ComputeType() const { CompileType LoadFieldInstr::ComputeType() const { if (slot().IsRecordField()) { + const intptr_t index = compiler::target::Record::field_index_at_offset( + slot().offset_in_bytes()); + if (auto* alloc = instance()->definition()->AsAllocateSmallRecord()) { + if (index < alloc->num_fields()) { + return *(alloc->InputAt(index)->Type()); + } + } const AbstractType* instance_type = instance()->Type()->ToAbstractType(); if (instance_type->IsRecordType()) { - const intptr_t index = compiler::target::Record::field_index_at_offset( - slot().offset_in_bytes()); const auto& field_type = AbstractType::ZoneHandle( RecordType::Cast(*instance_type).FieldTypeAt(index)); return CompileType::FromAbstractType(field_type, CompileType::kCanBeNull,