From c701e765061732323f8520e3b803ea26aac1e1bd Mon Sep 17 00:00:00 2001 From: Aart Bik Date: Thu, 17 Jan 2019 22:22:32 +0000 Subject: [PATCH] [vm/compiler] handle non-nullable null situation better Rationale: Previously, we avoided introducing redefinitions that introduced the empty non-nullable null type. This situation arises when we do a null check on an actual null value (making all subsequent uses effectively dead code). This is too simple, however, since it still allows hoisting the uses before the check. This CL gives a better solution by introducing redefinitions without a constraining type (which are not removed and avoid the type). In the long run perhaps the best solution would be to simply remove all subsequent uses as dead. https://github.com/dart-lang/sdk/issues/32167 https://github.com/dart-lang/sdk/issues/34473 https://github.com/dart-lang/sdk/issues/35335 Change-Id: Ib5dd072a9e546f6b91faa52ea08e8c0f6350d7e0 Reviewed-on: https://dart-review.googlesource.com/c/89922 Reviewed-by: Alexander Markov Commit-Queue: Aart Bik --- runtime/vm/compiler/backend/compile_type.h | 8 +++++++- runtime/vm/compiler/backend/flow_graph.cc | 8 +++++++- runtime/vm/compiler/backend/type_propagator.cc | 4 ++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/runtime/vm/compiler/backend/compile_type.h b/runtime/vm/compiler/backend/compile_type.h index 217ab1ebdbb..ded5fa606f4 100644 --- a/runtime/vm/compiler/backend/compile_type.h +++ b/runtime/vm/compiler/backend/compile_type.h @@ -80,7 +80,13 @@ class CompileType : public ZoneAllocated { // abstract type. The pair is assumed to be coherent. static CompileType Create(intptr_t cid, const AbstractType& type); - CompileType CopyNonNullable() const { + // Return the non-nullable version of this type. + CompileType CopyNonNullable() { + if (IsNull()) { + // Represent a non-nullable null type (typically arising for + // unreachable values) as None. + return None(); + } return CompileType(kNonNullable, kIllegalCid, type_); } diff --git a/runtime/vm/compiler/backend/flow_graph.cc b/runtime/vm/compiler/backend/flow_graph.cc index b5d2201fade..7062a357c4a 100644 --- a/runtime/vm/compiler/backend/flow_graph.cc +++ b/runtime/vm/compiler/backend/flow_graph.cc @@ -1559,7 +1559,13 @@ RedefinitionInstr* FlowGraph::EnsureRedefinition(Instruction* prev, } } RedefinitionInstr* redef = new RedefinitionInstr(new Value(original)); - redef->set_constrained_type(new CompileType(compile_type)); + + // Don't set the constrained type when the type is None(), which denotes an + // unreachable value (e.g. using value null after an explicit null check). + if (!compile_type.IsNone()) { + redef->set_constrained_type(new CompileType(compile_type)); + } + InsertAfter(prev, redef, NULL, FlowGraph::kValue); RenameDominatedUses(original, redef, redef); return redef; diff --git a/runtime/vm/compiler/backend/type_propagator.cc b/runtime/vm/compiler/backend/type_propagator.cc index cb7360eacb2..6e70d926ecb 100644 --- a/runtime/vm/compiler/backend/type_propagator.cc +++ b/runtime/vm/compiler/backend/type_propagator.cc @@ -283,7 +283,7 @@ void FlowGraphTypePropagator::VisitCheckClassId(CheckClassIdInstr* check) { void FlowGraphTypePropagator::VisitCheckNull(CheckNullInstr* check) { Definition* receiver = check->value()->definition(); CompileType* type = TypeOf(receiver); - if (type->is_nullable() && !type->IsNull()) { + if (type->is_nullable()) { // Insert redefinition for the receiver to guard against invalid // code motion. EnsureMoreAccurateRedefinition(check, receiver, type->CopyNonNullable()); @@ -305,7 +305,7 @@ void FlowGraphTypePropagator::CheckNonNullSelector( if (target.IsNull()) { // If the selector is not defined on Null, we can propagate non-nullness. CompileType* type = TypeOf(receiver); - if (type->is_nullable() && !type->IsNull()) { + if (type->is_nullable()) { // Insert redefinition for the receiver to guard against invalid // code motion. EnsureMoreAccurateRedefinition(call, receiver, type->CopyNonNullable());