Fix wrong type test optimization when testing against a signature class: we must always consider th…

runtime/vm/flow_graph_compiler_arm.cc

 // Copyright (c) 2013, 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.
 
 #include "vm/globals.h"  // Needed here to get TARGET_ARCH_ARM.
 #if defined(TARGET_ARCH_ARM)
 
 #include "vm/flow_graph_compiler.h"
 
 #include "lib/error.h"
@@ skipping 206 matching lines @@
 // Clobbers R2.
 RawSubtypeTestCache*
 FlowGraphCompiler::GenerateInstantiatedTypeWithArgumentsTest(
     intptr_t token_pos,
     const AbstractType& type,
     Label* is_instance_lbl,
     Label* is_not_instance_lbl) {
   __ Comment("InstantiatedTypeWithArgumentsTest");
   ASSERT(type.IsInstantiated());
   const Class& type_class = Class::ZoneHandle(type.type_class());
-  ASSERT(type_class.HasTypeArguments());
+  ASSERT(type_class.HasTypeArguments() || type_class.IsSignatureClass());
   const Register kInstanceReg = R0;
   Error& malformed_error = Error::Handle();
   const Type& int_type = Type::Handle(Type::IntType());
   const bool smi_is_ok = int_type.IsSubtypeOf(type, &malformed_error);
-  // Malforrmed type should have been handled at graph construction time.
+  // Malformed type should have been handled at graph construction time.
   ASSERT(smi_is_ok || malformed_error.IsNull());
   __ tst(kInstanceReg, ShifterOperand(kSmiTagMask));
   if (smi_is_ok) {
     __ b(is_instance_lbl, EQ);
   } else {
     __ b(is_not_instance_lbl, EQ);
   }
   const AbstractTypeArguments& type_arguments =
       AbstractTypeArguments::ZoneHandle(type.arguments());
   const bool is_raw_type = type_arguments.IsNull() ||
       type_arguments.IsRaw(type_arguments.Length());
-  if (is_raw_type) {
-    const Register kClassIdReg = R2;
-    // dynamic type argument, check only classes.
-    __ LoadClassId(kClassIdReg, kInstanceReg);
-    __ CompareImmediate(kClassIdReg, type_class.id());
-    __ b(is_instance_lbl, EQ);
-    // List is a very common case.
-    if (IsListClass(type_class)) {
-      GenerateListTypeCheck(kClassIdReg, is_instance_lbl);
+  if (!type_class.IsSignatureClass()) {

[regis, 2013/07/12 00:29:31]

A comment explaining why we do not do a class check for signature classes could
help in the future.

[srdjan, 2013/07/12 16:56:11]

Added comment:

Signature class is an instantiated parameterized type.

+    if (is_raw_type) {
+      const Register kClassIdReg = R2;
+      // dynamic type argument, check only classes.
+      __ LoadClassId(kClassIdReg, kInstanceReg);
+      __ CompareImmediate(kClassIdReg, type_class.id());
+      __ b(is_instance_lbl, EQ);
+      // List is a very common case.
+      if (IsListClass(type_class)) {
+        GenerateListTypeCheck(kClassIdReg, is_instance_lbl);
+      }
+      return GenerateSubtype1TestCacheLookup(
+          token_pos, type_class, is_instance_lbl, is_not_instance_lbl);
     }
-    return GenerateSubtype1TestCacheLookup(
-        token_pos, type_class, is_instance_lbl, is_not_instance_lbl);
-  }
-  // If one type argument only, check if type argument is Object or dynamic.
-  if (type_arguments.Length() == 1) {
-    const AbstractType& tp_argument = AbstractType::ZoneHandle(
-        type_arguments.TypeAt(0));
-    ASSERT(!tp_argument.IsMalformed());
-    if (tp_argument.IsType()) {
-      ASSERT(tp_argument.HasResolvedTypeClass());
-      // Check if type argument is dynamic or Object.
-      const Type& object_type = Type::Handle(Type::ObjectType());
-      if (object_type.IsSubtypeOf(tp_argument, NULL)) {
-        // Instance class test only necessary.
-        return GenerateSubtype1TestCacheLookup(
-            token_pos, type_class, is_instance_lbl, is_not_instance_lbl);
+    // If one type argument only, check if type argument is Object or dynamic.
+    if (type_arguments.Length() == 1) {
+      const AbstractType& tp_argument = AbstractType::ZoneHandle(
+          type_arguments.TypeAt(0));
+      ASSERT(!tp_argument.IsMalformed());
+      if (tp_argument.IsType()) {
+        ASSERT(tp_argument.HasResolvedTypeClass());
+        // Check if type argument is dynamic or Object.
+        const Type& object_type = Type::Handle(Type::ObjectType());
+        if (object_type.IsSubtypeOf(tp_argument, NULL)) {
+          // Instance class test only necessary.
+          return GenerateSubtype1TestCacheLookup(
+              token_pos, type_class, is_instance_lbl, is_not_instance_lbl);
+        }
       }
     }
   }
   // Regular subtype test cache involving instance's type arguments.
   const Register kTypeArgumentsReg = kNoRegister;
   const Register kTempReg = kNoRegister;
   // R0: instance (must be preserved).
   return GenerateCallSubtypeTestStub(kTestTypeTwoArgs,
                                      kInstanceReg,
                                      kTypeArgumentsReg,
@@ skipping 225 matching lines @@
       __ b(is_not_instance_lbl, EQ);
       __ CompareClassId(kInstanceReg, type_cid, R3);
       __ b(is_instance_lbl, EQ);
     }
     __ b(is_not_instance_lbl);
     return SubtypeTestCache::null();
   }
   if (type.IsInstantiated()) {
     const Class& type_class = Class::ZoneHandle(type.type_class());
     // A class equality check is only applicable with a dst type of a
     // non-parameterized class or with a raw dst type of a parameterized class.

[regis, 2013/07/12 00:29:31]

Should this comment be updated?

[srdjan, 2013/07/12 16:56:11]

Added:
non-signature class

-    if (type_class.HasTypeArguments()) {
+    if (type_class.IsSignatureClass() || type_class.HasTypeArguments()) {
       return GenerateInstantiatedTypeWithArgumentsTest(token_pos,
                                                        type,
                                                        is_instance_lbl,
                                                        is_not_instance_lbl);
       // Fall through to runtime call.
     }
     const bool has_fall_through =
         GenerateInstantiatedTypeNoArgumentsTest(token_pos,
                                                 type,
                                                 is_instance_lbl,
@@ skipping 1317 matching lines @@
 void ParallelMoveResolver::RestoreFpuScratch(FpuRegister reg) {
   __ vldrd(reg, Address(SP, kDoubleSize, Address::PostIndex));
 }
 
 
 #undef __
 
 }  // namespace dart
 
 #endif  // defined TARGET_ARCH_ARM