The VM needs to account for super-bounded types (supertypes of the instantiated-to-bounds type) in type check optimizations #52848
Comments
sstrickl
added
the
area-front-end
|
/cc @johnniwinther @chloestefantsova for the CFE |
Not quite, IIUC. We do not maintain a constraint that every type in a Dart program is an error unless every declared type parameter bound is satisfied (we use the phrase regular-bounded to characterize a type of the form This kind of type is known as a super-bounded type, cf. https://github.com/dart-lang/language/blob/ccadba7019d3f39885c9652011de02a8f6879ed2/specification/dartLangSpec.tex#L7211. I would assume that the CFE will reflect this language-level rule: The CFE would then only generate regular-bounded "types" when an expression like A super-bounded type is, roughly, a type where all bounds have been satisfied, except that some subterms can denote a top type (like class C<X extends num> {}
class D<X extends D<X>> {}
void foo(C<Object?> c, D<D<dynamic>> d) {} // OK.
C<Object> get g => throw 0; // Error, `C<Object>` is not well-bounded.Instantiation to bounds can yield a super-bounded type: |
|
Okay, thanks for the clarification, Erik! I think that means the change in 7cc005e is fine and valid, but there may be cases where the CFE may provide a super-bounded type that we should treat as if it were the instantiated-to-bounds type (e.g., optimization of checking the runtime type of instances), and so I should do a followup to make that change. I'll leave this issue open, but move it over to my side of the pond :) |
sstrickl
added
the
area-vm
sstrickl
changed the title
sstrickl
removed
the
area-front-end
|
Sounds good, thanks! (Also, I'm sure it would still be useful if @johnniwinther or @chloestefantsova could clarify how the CFE has chosen to handle super-bounded types, and if there's anything extra to think about). |
|
I should mention that the language specification section about super-bounded types specifies pre-nnbd rules. The rules for the language with null safety are under review in dart-lang/language#2605. |
Previously, the FFI transformer could produce is checks where the type to check against was Pointer<dynamic>. However, given that the Pointer class is defined as: abstract class Pointer<X extends NativeType> ... the instantiated to bounds version of its type is Pointer<NativeType>. Pointer<dynamic> is not a subtype of Pointer<NativeType>, and thus is an invalid instantiation, but the only place this type could occur was as the right hand side of an is check. Before 7cc005e, Class::RareType() returned the class instantiated with the null (all-dynamic) type arguments vector. Among other things, this "rare" type was compared to the right-hand side of is checks and, if it matched, performed a simple (cid-only) check of the instance type arguments in unoptimized code. Afterwards, Class::RareType() returns the class instantiated with a type arguments vector where each type parameter is instantiated to bounds, so now the "rare" type check fails and it falls back to the full check of the instance type arguments, which causes a ~25% regression in some unoptimized benchmarks. This CL fixes the generation of those is checks in the FFI transformer to use the instantiated to bounds version of the Pointer type instead. TEST=pkg/front_end/test Issue: #52843 Issue: #52848 Cq-Include-Trybots: luci.dart.try:vm-ffi-qemu-linux-release-riscv64-try,vm-ffi-qemu-linux-release-arm-try,vm-aot-linux-debug-x64-try,vm-linux-debug-x64-try Change-Id: Ic9ac6d75ba2743e233065444fad13ab098094349 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312400 Reviewed-by: Daco Harkes <dacoharkes@google.com> Reviewed-by: Erik Ernst <eernst@google.com> Auto-Submit: Tess Strickland <sstrickl@google.com> Commit-Queue: Tess Strickland <sstrickl@google.com>
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 <kustermann@google.com> Commit-Queue: Tess Strickland <sstrickl@google.com>
We do that like you described: we retain the super-bounded types written by the programmer and we may generate a super-bounded type as the output of the instantiate-to-bound algorithm. |
a-siva
added
triaged
|
@sstrickl is this still a P2 issue or should we make it P3 ? |
a-siva
added
P3
When developing CL 312400, I originally put a check in the VM when in DEBUG mode that verified that each type argument for an
InterfaceTyperead from kernel is a subtype of the corresponding type argument when the class type parameters were instantiated to bounds.However, that check turned out to be too strong for the CFE as it stands currently, as the CFE can produce kernel where this assumption is broken (from a failing build):
I then introduced a weaker check, that only checked that fully instantiated types read from kernel were subtypes of the type where the class is instantiated to bounds. This allowed the build to succeed, but that check failed on some tests (see this example from the log for one of the failing trybots):
So currently there are cases where the CFE can generate raw (all type arguments are dynamic) or partially raw types for classes where the (partially) raw type is an invalid instantiation of the class. I'm guessing there's a similar issue with
TypedefTypes as well, though I haven't verified that on the VM side.Should the CFE provide a guarantee that all types generated by the CFE are valid subtypes of the instantiated to bounds type?
The text was updated successfully, but these errors were encountered: