Skip to content

Commit

Permalink
Reland "[VM runtime] Support Smi instances in type test cache."
Browse files Browse the repository at this point in the history 
This is a reland of 6ba3e55

The issue was that SlowTypeTestStub used in precompiled mode did not handle a
Smi instance before calling the Subtype2TestCache stub which does not support
it. See PatchSet 2 for the fix.
Is there a more efficient solution?

Original change's description:
> [VM runtime] Support Smi instances in type test cache.
>
> This adds SubtypeTestCache-based optimizations for type tests against
>       * dst_type = FutureOr<T> (when T=int/num)
>       * dst_type = T (when T = FutureOr<int/num>)
>
> Remove dangerous LoadClass pseudo assembler instruction (does not work for Smi).
> Handle instantiated void in type tests (along with dynamic and Object).
>
> Change-Id: I0df0fc72ff173b9464d16cc971969132b055a429
> Reviewed-on: https://dart-review.googlesource.com/c/81182
> Commit-Queue: Régis Crelier <regis@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>

Change-Id: I333ca47aebd7f0b663059ab6afc5d1cd8d7d5210
Reviewed-on: https://dart-review.googlesource.com/c/81320
Commit-Queue: Régis Crelier <regis@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
  • Loading branch information
@crelier
crelier authored and commit-bot@chromium.org committed Oct 24, 2018
1 parent 3dc9119 commit 11ad25a
Show file tree
Hide file tree
Showing 18 changed files with 150 additions and 123 deletions.
2 changes: 1 addition & 1 deletion runtime/vm/compiler/aot/precompiler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ void Precompiler::AddConstObject(const Instance& instance) {
return;
}

// Can't ask immediate objects if they're canoncial.
// Can't ask immediate objects if they're canonical.
if (instance.IsSmi()) return;

// Some Instances in the ObjectPool aren't const objects, such as
Expand Down
6 changes: 0 additions & 6 deletions runtime/vm/compiler/assembler/assembler_arm.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1825,12 +1825,6 @@ void Assembler::LoadClassById(Register result, Register class_id) {
ldr(result, Address(result, class_id, LSL, kSizeOfClassPairLog2));
}

void Assembler::LoadClass(Register result, Register object, Register scratch) {
ASSERT(scratch != result);
LoadClassId(scratch, object);
LoadClassById(result, scratch);
}

void Assembler::CompareClassId(Register object,
intptr_t class_id,
Register scratch) {
Expand Down
1 change: 0 additions & 1 deletion runtime/vm/compiler/assembler/assembler_arm.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,6 @@ class Assembler : public ValueObject {

void LoadClassId(Register result, Register object, Condition cond = AL);
void LoadClassById(Register result, Register class_id);
void LoadClass(Register result, Register object, Register scratch);
void CompareClassId(Register object, intptr_t class_id, Register scratch);
void LoadClassIdMayBeSmi(Register result, Register object);
void LoadTaggedClassIdMayBeSmi(Register result, Register object);
Expand Down
6 changes: 0 additions & 6 deletions runtime/vm/compiler/assembler/assembler_arm64.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1115,12 +1115,6 @@ void Assembler::LoadClassById(Register result, Register class_id) {
ldr(result, Address(result, class_id, UXTX, Address::Scaled));
}

void Assembler::LoadClass(Register result, Register object) {
ASSERT(object != TMP);
LoadClassId(TMP, object);
LoadClassById(result, TMP);
}

void Assembler::CompareClassId(Register object,
intptr_t class_id,
Register scratch) {
Expand Down
1 change: 0 additions & 1 deletion runtime/vm/compiler/assembler/assembler_arm64.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1531,7 +1531,6 @@ class Assembler : public ValueObject {
void LoadClassId(Register result, Register object);
// Overwrites class_id register (it will be tagged afterwards).
void LoadClassById(Register result, Register class_id);
void LoadClass(Register result, Register object);
void CompareClassId(Register object,
intptr_t class_id,
Register scratch = kNoRegister);
Expand Down
6 changes: 0 additions & 6 deletions runtime/vm/compiler/assembler/assembler_ia32.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2434,12 +2434,6 @@ void Assembler::LoadClassById(Register result, Register class_id) {
movl(result, Address(result, class_id, TIMES_8, 0));
}

void Assembler::LoadClass(Register result, Register object, Register scratch) {
ASSERT(scratch != result);
LoadClassId(scratch, object);
LoadClassById(result, scratch);
}

void Assembler::CompareClassId(Register object,
intptr_t class_id,
Register scratch) {
Expand Down
2 changes: 0 additions & 2 deletions runtime/vm/compiler/assembler/assembler_ia32.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -666,8 +666,6 @@ class Assembler : public ValueObject {

void LoadClassById(Register result, Register class_id);

void LoadClass(Register result, Register object, Register scratch);

void CompareClassId(Register object, intptr_t class_id, Register scratch);

void LoadClassIdMayBeSmi(Register result, Register object);
Expand Down
5 changes: 0 additions & 5 deletions runtime/vm/compiler/assembler/assembler_x64.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1952,11 +1952,6 @@ void Assembler::LoadClassById(Register result, Register class_id) {
movq(result, Address(result, class_id, TIMES_8, 0));
}

void Assembler::LoadClass(Register result, Register object) {
LoadClassId(TMP, object);
LoadClassById(result, TMP);
}

void Assembler::CompareClassId(Register object,
intptr_t class_id,
Register scratch) {
Expand Down
2 changes: 0 additions & 2 deletions runtime/vm/compiler/assembler/assembler_x64.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -782,8 +782,6 @@ class Assembler : public ValueObject {
// Overwrites class_id register (it will be tagged afterwards).
void LoadClassById(Register result, Register class_id);

void LoadClass(Register result, Register object);

void CompareClassId(Register object,
intptr_t class_id,
Register scratch = kNoRegister);
Expand Down
34 changes: 22 additions & 12 deletions runtime/vm/compiler/backend/flow_graph_compiler_arm.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,13 +248,14 @@ FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
ASSERT(type_class.NumTypeArguments() > 0);
const Register kInstanceReg = R0;
Error& bound_error = Error::Handle(zone());
const Type& int_type = Type::Handle(zone(), Type::IntType());
const Type& smi_type = Type::Handle(zone(), Type::SmiType());
const bool smi_is_ok =
int_type.IsSubtypeOf(type, &bound_error, NULL, Heap::kOld);
smi_type.IsSubtypeOf(type, &bound_error, NULL, Heap::kOld);
// Malformed type should have been handled at graph construction time.
ASSERT(smi_is_ok || bound_error.IsNull());
__ tst(kInstanceReg, Operand(kSmiTagMask));
if (smi_is_ok) {
// Fast case for type = FutureOr<int/num/top-type>.
__ b(is_instance_lbl, EQ);
} else {
__ b(is_not_instance_lbl, EQ);
Expand Down Expand Up @@ -286,7 +287,7 @@ FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
ASSERT(!tp_argument.IsMalformed());
if (tp_argument.IsType()) {
ASSERT(tp_argument.HasResolvedTypeClass());
// Check if type argument is dynamic or Object.
// Check if type argument is dynamic, Object, or void.
const Type& object_type = Type::Handle(zone(), Type::ObjectType());
if (object_type.IsSubtypeOf(tp_argument, NULL, NULL, Heap::kOld)) {
// Instance class test only necessary.
Expand Down Expand Up @@ -341,6 +342,7 @@ bool FlowGraphCompiler::GenerateInstantiatedTypeNoArgumentsTest(
if (smi_class.IsSubtypeOf(Object::null_type_arguments(), type_class,
Object::null_type_arguments(), NULL, NULL,
Heap::kOld)) {
// Fast case for type = int/num/top-type.
__ b(is_instance_lbl, EQ);
} else {
__ b(is_not_instance_lbl, EQ);
Expand Down Expand Up @@ -398,7 +400,14 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup(
Label* is_not_instance_lbl) {
__ Comment("Subtype1TestCacheLookup");
const Register kInstanceReg = R0;
__ LoadClass(R1, kInstanceReg, R2);
#if defined(DEBUG)
Label ok;
__ BranchIfNotSmi(kInstanceReg, &ok);
__ Breakpoint();
__ Bind(&ok);
#endif
__ LoadClassId(R2, kInstanceReg);
__ LoadClassById(R1, R2);
// R1: instance class.
// Check immediate superclass equality.
__ ldr(R2, FieldAddress(R1, Class::super_type_offset()));
Expand Down Expand Up @@ -447,12 +456,13 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
__ ldr(R3, FieldAddress(kTypeArgumentsReg,
TypeArguments::type_at_offset(type_param.index())));
// R3: concrete type of type.
// Check if type argument is dynamic.
// Check if type argument is dynamic, Object, or void.
__ CompareObject(R3, Object::dynamic_type());
__ b(is_instance_lbl, EQ);
__ CompareObject(R3, Type::ZoneHandle(zone(), Type::ObjectType()));
__ b(is_instance_lbl, EQ);
// TODO(regis): Optimize void type as well once allowed as type argument.
__ CompareObject(R3, Object::void_type());
__ b(is_instance_lbl, EQ);

// For Smi check quickly against int and num interfaces.
Label not_smi;
Expand All @@ -462,9 +472,8 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
__ b(is_instance_lbl, EQ);
__ CompareObject(R3, Type::ZoneHandle(zone(), Type::Number()));
__ b(is_instance_lbl, EQ);
// Smi must be handled in runtime.
Label fall_through;
__ b(&fall_through);
// Smi can be handled by type test cache.
__ Bind(&not_smi);

// If it's guaranteed, by type-parameter bound, that the type parameter will
// never have a value of a function type, then we can safely do a 4-type
Expand All @@ -474,17 +483,18 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
? kTestTypeSixArgs
: kTestTypeFourArgs;

__ Bind(&not_smi);
const SubtypeTestCache& type_test_cache = SubtypeTestCache::ZoneHandle(
zone(), GenerateCallSubtypeTestStub(
test_kind, kInstanceReg, kInstantiatorTypeArgumentsReg,
kFunctionTypeArgumentsReg, kTempReg, is_instance_lbl,
is_not_instance_lbl));
__ Bind(&fall_through);
return type_test_cache.raw();
}
if (type.IsType()) {
__ BranchIfSmi(kInstanceReg, is_not_instance_lbl);
// Smi is FutureOr<T>, when T is a top type or int or num.
if (!FLAG_strong || !Class::Handle(type.type_class()).IsFutureOrClass()) {
__ BranchIfSmi(kInstanceReg, is_not_instance_lbl);
}
__ ldm(IA, SP,
(1 << kFunctionTypeArgumentsReg) |
(1 << kInstantiatorTypeArgumentsReg));
Expand Down
34 changes: 22 additions & 12 deletions runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,12 @@ FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
ASSERT(type_class.NumTypeArguments() > 0);
const Register kInstanceReg = R0;
Error& bound_error = Error::Handle(zone());
const Type& int_type = Type::Handle(zone(), Type::IntType());
const Type& smi_type = Type::Handle(zone(), Type::SmiType());
const bool smi_is_ok =
int_type.IsSubtypeOf(type, &bound_error, NULL, Heap::kOld);
smi_type.IsSubtypeOf(type, &bound_error, NULL, Heap::kOld);
// Malformed type should have been handled at graph construction time.
ASSERT(smi_is_ok || bound_error.IsNull());
// Fast case for type = FutureOr<int/num/top-type>.
__ BranchIfSmi(kInstanceReg,
smi_is_ok ? is_instance_lbl : is_not_instance_lbl);

Expand Down Expand Up @@ -278,7 +279,7 @@ FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
ASSERT(!tp_argument.IsMalformed());
if (tp_argument.IsType()) {
ASSERT(tp_argument.HasResolvedTypeClass());
// Check if type argument is dynamic or Object.
// Check if type argument is dynamic, Object, or void.
const Type& object_type = Type::Handle(zone(), Type::ObjectType());
if (object_type.IsSubtypeOf(tp_argument, NULL, NULL, Heap::kOld)) {
// Instance class test only necessary.
Expand Down Expand Up @@ -331,6 +332,7 @@ bool FlowGraphCompiler::GenerateInstantiatedTypeNoArgumentsTest(
if (smi_class.IsSubtypeOf(Object::null_type_arguments(), type_class,
Object::null_type_arguments(), NULL, NULL,
Heap::kOld)) {
// Fast case for type = int/num/top-type.
__ BranchIfSmi(kInstanceReg, is_instance_lbl);
} else {
__ BranchIfSmi(kInstanceReg, is_not_instance_lbl);
Expand Down Expand Up @@ -388,7 +390,14 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup(
Label* is_not_instance_lbl) {
__ Comment("Subtype1TestCacheLookup");
const Register kInstanceReg = R0;
__ LoadClass(R1, kInstanceReg);
#if defined(DEBUG)
Label ok;
__ BranchIfNotSmi(kInstanceReg, &ok);
__ Breakpoint();
__ Bind(&ok);
#endif
__ LoadClassId(TMP, kInstanceReg);
__ LoadClassById(R1, TMP);
// R1: instance class.
// Check immediate superclass equality.
__ LoadFieldFromOffset(R2, R1, Class::super_type_offset());
Expand Down Expand Up @@ -435,12 +444,13 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
__ LoadFieldFromOffset(R3, kTypeArgumentsReg,
TypeArguments::type_at_offset(type_param.index()));
// R3: concrete type of type.
// Check if type argument is dynamic.
// Check if type argument is dynamic, Object, or void.
__ CompareObject(R3, Object::dynamic_type());
__ b(is_instance_lbl, EQ);
__ CompareObject(R3, Type::ZoneHandle(zone(), Type::ObjectType()));
__ b(is_instance_lbl, EQ);
// TODO(regis): Optimize void type as well once allowed as type argument.
__ CompareObject(R3, Object::void_type());
__ b(is_instance_lbl, EQ);

// For Smi check quickly against int and num interfaces.
Label not_smi;
Expand All @@ -449,9 +459,8 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
__ b(is_instance_lbl, EQ);
__ CompareObject(R3, Type::ZoneHandle(zone(), Type::Number()));
__ b(is_instance_lbl, EQ);
// Smi must be handled in runtime.
Label fall_through;
__ b(&fall_through);
// Smi can be handled by type test cache.
__ Bind(&not_smi);

// If it's guaranteed, by type-parameter bound, that the type parameter will
// never have a value of a function type, then we can safely do a 4-type
Expand All @@ -461,17 +470,18 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
? kTestTypeSixArgs
: kTestTypeFourArgs;

__ Bind(&not_smi);
const SubtypeTestCache& type_test_cache = SubtypeTestCache::ZoneHandle(
zone(), GenerateCallSubtypeTestStub(
test_kind, kInstanceReg, kInstantiatorTypeArgumentsReg,
kFunctionTypeArgumentsReg, kTempReg, is_instance_lbl,
is_not_instance_lbl));
__ Bind(&fall_through);
return type_test_cache.raw();
}
if (type.IsType()) {
__ BranchIfSmi(kInstanceReg, is_not_instance_lbl);
// Smi is FutureOr<T>, when T is a top type or int or num.
if (!FLAG_strong || !Class::Handle(type.type_class()).IsFutureOrClass()) {
__ BranchIfSmi(kInstanceReg, is_not_instance_lbl);
}
__ ldp(kFunctionTypeArgumentsReg, kInstantiatorTypeArgumentsReg,
Address(SP, 0 * kWordSize, Address::PairOffset));
// Uninstantiated type class is known at compile time, but the type
Expand Down
36 changes: 23 additions & 13 deletions runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,13 +256,14 @@ FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
ASSERT(type_class.NumTypeArguments() > 0);
const Register kInstanceReg = EAX;
Error& bound_error = Error::Handle(zone());
const Type& int_type = Type::Handle(zone(), Type::IntType());
const Type& smi_type = Type::Handle(zone(), Type::SmiType());
const bool smi_is_ok =
int_type.IsSubtypeOf(type, &bound_error, NULL, Heap::kOld);
smi_type.IsSubtypeOf(type, &bound_error, NULL, Heap::kOld);
// Malformed type should have been handled at graph construction time.
ASSERT(smi_is_ok || bound_error.IsNull());
__ testl(kInstanceReg, Immediate(kSmiTagMask));
if (smi_is_ok) {
// Fast case for type = FutureOr<int/num/top-type>.
__ j(ZERO, is_instance_lbl);
} else {
__ j(ZERO, is_not_instance_lbl);
Expand Down Expand Up @@ -294,7 +295,7 @@ FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
ASSERT(!tp_argument.IsMalformed());
if (tp_argument.IsType()) {
ASSERT(tp_argument.HasResolvedTypeClass());
// Check if type argument is dynamic or Object.
// Check if type argument is dynamic, Object, or void.
const Type& object_type = Type::Handle(zone(), Type::ObjectType());
if (object_type.IsSubtypeOf(tp_argument, NULL, NULL, Heap::kOld)) {
// Instance class test only necessary.
Expand Down Expand Up @@ -347,6 +348,7 @@ bool FlowGraphCompiler::GenerateInstantiatedTypeNoArgumentsTest(
if (smi_class.IsSubtypeOf(Object::null_type_arguments(), type_class,
Object::null_type_arguments(), NULL, NULL,
Heap::kOld)) {
// Fast case for type = int/num/top-type.
__ j(ZERO, is_instance_lbl);
} else {
__ j(ZERO, is_not_instance_lbl);
Expand Down Expand Up @@ -404,7 +406,14 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup(
Label* is_not_instance_lbl) {
__ Comment("Subtype1TestCacheLookup");
const Register kInstanceReg = EAX;
__ LoadClass(ECX, kInstanceReg, EDI);
#if defined(DEBUG)
Label ok;
__ BranchIfNotSmi(kInstanceReg, &ok);
__ Breakpoint();
__ Bind(&ok);
#endif
__ LoadClassId(EDI, kInstanceReg);
__ LoadClassById(ECX, EDI);
// ECX: instance class.
// Check immediate superclass equality.
__ movl(EDI, FieldAddress(ECX, Class::super_type_offset()));
Expand Down Expand Up @@ -455,12 +464,13 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
__ movl(EDI, FieldAddress(kTypeArgumentsReg, TypeArguments::type_at_offset(
type_param.index())));
// EDI: concrete type of type.
// Check if type argument is dynamic.
// Check if type argument is dynamic, Object, or void.
__ CompareObject(EDI, Object::dynamic_type());
__ j(EQUAL, is_instance_lbl);
__ CompareObject(EDI, Type::ZoneHandle(zone(), Type::ObjectType()));
__ j(EQUAL, is_instance_lbl);
// TODO(regis): Optimize void type as well once allowed as type argument.
__ CompareObject(EDI, Object::void_type());
__ j(EQUAL, is_instance_lbl);

// For Smi check quickly against int and num interfaces.
Label not_smi;
Expand All @@ -470,9 +480,8 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
__ j(EQUAL, is_instance_lbl);
__ CompareObject(EDI, Type::ZoneHandle(zone(), Type::Number()));
__ j(EQUAL, is_instance_lbl);
// Smi must be handled in runtime.
Label fall_through;
__ jmp(&fall_through);
// Smi can be handled by type test cache.
__ Bind(&not_smi);

// If it's guaranteed, by type-parameter bound, that the type parameter will
// never have a value of a function type.
Expand All @@ -481,18 +490,19 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateUninstantiatedTypeTest(
? kTestTypeSixArgs
: kTestTypeFourArgs;

__ Bind(&not_smi);
const SubtypeTestCache& type_test_cache = SubtypeTestCache::ZoneHandle(
zone(), GenerateCallSubtypeTestStub(
test_kind, kInstanceReg, kInstantiatorTypeArgumentsReg,
kFunctionTypeArgumentsReg, kTempReg, is_instance_lbl,
is_not_instance_lbl));
__ Bind(&fall_through);
return type_test_cache.raw();
}
if (type.IsType()) {
__ testl(kInstanceReg, Immediate(kSmiTagMask)); // Is instance Smi?
__ j(ZERO, is_not_instance_lbl);
// Smi is FutureOr<T>, when T is a top type or int or num.
if (!FLAG_strong || !Class::Handle(type.type_class()).IsFutureOrClass()) {
__ testl(kInstanceReg, Immediate(kSmiTagMask)); // Is instance Smi?
__ j(ZERO, is_not_instance_lbl);
}
__ movl(kInstantiatorTypeArgumentsReg, Address(ESP, 1 * kWordSize));
__ movl(kFunctionTypeArgumentsReg, Address(ESP, 0 * kWordSize));
// Uninstantiated type class is known at compile time, but the type
Expand Down
Loading

0 comments on commit 11ad25a

Please sign in to comment.