[vm] Tweak handling of rare types with non-null instance type arguments.

The CFE may return super-bounded types (types that are a strict
supertype of the rare type) for certain type positions like the right
hand sides of `is` and `as` checks, so we can't assume that all types
involving a given class is a subtype of its rare type. Note that this is
only for uses that do not involve instantiation, as all instances of a
class must have runtime types that are a subtype of its rare type.

Keeping this in mind, here are the changes being made in this CL.

-----

In the flow graph builder when producting unoptimized code, we
previously assumed that if the checked type is not equal to the rare
type, then the code can't use _simpleInstanceOf, which only performs
class ID checks.  However, if the checked type is a supertype of the
rare type, we can, because any instance is guaranteed to be a subtype of
the checked type.

Note that this didn't cause incorrect behavior, just more work than
needed in some cases.

-----

In the generation of optimized type testing stubs, we assumed that the
null type argument vector (TAV) is a possible instance TAV for any
instance. However, if the rare type for a class has a non-null TAV, then
no instance of that class can have a null TAV and we can skip the null
check. (We keep it in the DEBUG case, but just to immediately trigger
a breakpoint if seen.)

Again, no incorrect behavior caused here previously, just an unnecessary
check that we now remove in some cases.

------

Also when generating optimized type testing stubs, when checking for the
null TAV prior to instance type argument checks, the code assumed that
it was possible to have to check type arguments if the checked type was
a super type of the rare type for the instance's class.  However, that's
backwards: if the checked type is a super type, then the runtime type of
any instance of the class is guaranteed to be a subtype and we shouldn't
be checking type arguments at all.

Thus, add an ASSERT that checks this, and instead only generate the code
that goes to the runtime if the null TAV is seen, as the runtime type of
the instance is the rare type and the rare type is guaranteed to not be
a subtype of the checked type.

Again, the previous version of this wouldn't have caused incorrect
behavior either, because the VM should never generate type arguments
checking in the type testing stub if the checked type is a supertype of
the class's rare type. Thus, that branch in the code generation was
just dead code.

-----

TEST=vm/cc/TTS, ci (especially DEBUG bots)

Issue: #52848
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-debug-x64-try,vm-linux-debug-x64-try
Change-Id: I86d159693d6218f88dd1f04dd34cba702744b4d2
Fixed: 52848
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312500
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Tess Strickland <[email protected]>

Author: sstrickl

runtime/vm/compiler/frontend/flow_graph_builder.cc

@@ -380,17 +380,18 @@ bool SimpleInstanceOfType(const AbstractType& type) {
   }
 
   ASSERT(type.HasTypeClass());
-  const Class& type_class = Class::Handle(type.type_class());
+  auto* const zone = Thread::Current()->zone();
+  const Class& type_class = Class::Handle(zone, type.type_class());
 
   // Bail if the type has any type parameters.
   if (type_class.IsGeneric()) {
-    // If the interface type we check against is generic but has all-dynamic
-    // type arguments, then we can still use the _simpleInstanceOf
-    // implementation (see also runtime/lib/object.cc:Object_SimpleInstanceOf).
-    const auto& rare_type = AbstractType::Handle(type_class.RareType());
-    // TODO(regis): Revisit the usage of TypeEquality::kSyntactical when
-    // implementing strong mode.
-    return rare_type.IsEquivalent(type, TypeEquality::kSyntactical);
+    // If the interface type is a supertype of the rare type, as any instances
+    // are guaranteed to be subtypes of the rare type, then we can still use
+    // the _simpleInstanceOf implementation (see also
+    // runtime/lib/object.cc:Object_SimpleInstanceOf).
+    // See #52848 for why the type might be a supertype and not strictly equal.
+    const auto& rare_type = Type::Handle(zone, type_class.RareType());
+    return rare_type.IsSubtypeOf(type, Heap::kNew);
   }
 
   // Finally a simple class for instance of checking.

runtime/vm/type_testing_stubs.cc

@@ -619,25 +619,39 @@ void TypeTestingStubGenerator::
     // Only build type argument checking if any checked cid ranges require it.
     __ Bind(&load_succeeded);
 
-    // b) We check for "rare" types, where the instance type arguments are null.
-    //
-    // The kernel frontend should fill in any non-assigned type parameters on
-    // construction with dynamic/Object, so we should never get the null type
-    // argument vector in created instances.
-    //
-    // TODO(kustermann): We could consider not using "null" as type argument
-    // vector representing all-dynamic to avoid this extra check (which will be
-    // uncommon because most Dart code in 2.0 will be strongly typed)!
-    __ CompareObject(TTSInternalRegs::kInstanceTypeArgumentsReg,
-                     Object::null_object());
+    // The rare type of the class is guaranteed to be a supertype of the
+    // runtime type of any instance..
     const Type& rare_type = Type::Handle(type_class.RareType());
-    if (rare_type.IsSubtypeOf(type, Heap::kNew)) {
-      compiler::Label process_done;
-      __ BranchIf(NOT_EQUAL, &process_done, compiler::Assembler::kNearJump);
-      __ Ret();
-      __ Bind(&process_done);
-    } else {
+    // If the rare type is a subtype of the type being checked, then the runtime
+    // type of the instance is also a subtype and we shouldn't need to perform
+    // checks for the instance type arguments.
+    ASSERT(!rare_type.IsSubtypeOf(type, Heap::kNew));
+    // b) We check if the type arguments of the rare type are all dynamic
+    // (that is, the type arguments vector is null).
+    if (rare_type.arguments() == TypeArguments::null()) {
+      // If it is, then the instance could have a null instance TAV. However,
+      // if the instance TAV is null, then the runtime type of the instance is
+      // the rare type, which means it cannot be a subtype of the checked type.
+      __ CompareObject(TTSInternalRegs::kInstanceTypeArgumentsReg,
+                       Object::null_object());
       __ BranchIf(EQUAL, &check_failed);
+    } else {
+      // If the TAV of the rare type is not null, at least one type argument
+      // of the rare type is a non-top type. This means no instance can have
+      // a null instance TAV, as the dynamic type cannot be a subtype of
+      // a non-top type and each type argument of an instance must be
+      // a subtype of the corresponding type argument for the rare type.
+#if defined(DEBUG)
+      // Add the check for null in DEBUG mode, but instead of failing, create a
+      // breakpoint to make it obvious that the assumption above has failed.
+      __ CompareObject(TTSInternalRegs::kInstanceTypeArgumentsReg,
+                       Object::null_object());
+      compiler::Label check_instance_tav;
+      __ BranchIf(NOT_EQUAL, &check_instance_tav,
+                  compiler::Assembler::kNearJump);
+      __ Breakpoint();
+      __ Bind(&check_instance_tav);
+#endif
     }
 
     // c) Then we'll check each value of the type argument.