From 0bff5652bacee77f6b58923e6ed47b6e294adaa8 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Thu, 18 May 2023 18:33:37 +0000 Subject: [PATCH] Reland "[vm/compiler] Remove unnecessary type check from optimized switches" This is a reland of commit 43ce5487c1d72af27e9d8157e8d664dabcd7a9d7 In addition to removing type check, switch range checks are adjusted in order to avoid call to 'int.operator<=', which can be removed by tree shaker in AOT/product mode, causing NoSuchMethodError at runtime. Original change's description: > [vm/compiler] Remove unnecessary type check from optimized switches > > Switch optimization now relies on static type of the tested value, > so it is safe to omit the type check (with sound null safety) or > reduce it to a null check (without sound null safety). > > TEST=co19/LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_enum_A01_t01 > Fixes https://github.com/dart-lang/sdk/issues/52422 > > Change-Id: Ic93f4f212bee9ed3bfe5035f3c8d7535274c2f63 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/304102 > Commit-Queue: Alexander Markov > Reviewed-by: Alexander Aprelev TEST=co19/LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_enum_A01_t01 TEST=language/async/switch_test Change-Id: I70bf480bf376f946d66caa504120c8f85093ea8d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/304322 Reviewed-by: Alexander Aprelev Commit-Queue: Alexander Markov --- .../frontend/kernel_binary_flowgraph.cc | 57 +++++++------------ runtime/vm/compiler/frontend/kernel_to_il.cc | 33 +++++++++++ runtime/vm/compiler/frontend/kernel_to_il.h | 3 + 3 files changed, 58 insertions(+), 35 deletions(-) diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc index 28a7a397e86..cd2c4d4403e 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc @@ -5072,30 +5072,23 @@ Fragment StreamingFlowGraphBuilder::BuildOptimizedSwitchPrelude( SwitchHelper* helper, JoinEntryInstr* join) { const TokenPosition pos = helper->position(); - - // We need to check that the switch variable is of the correct type. - // If it is not, we go to [join] which is either the default case or - // the exit of the switch statement. - - TargetEntryInstr* then_entry; - TargetEntryInstr* otherwise_entry; - - const AbstractType& expression_type = helper->expression_type(); - ASSERT(dart::SimpleInstanceOfType(expression_type)); - Fragment instructions; - instructions += LoadLocal(scopes()->switch_variable); - instructions += Constant(expression_type); - instructions += InstanceCall( - pos, Library::PrivateCoreLibName(Symbols::_simpleInstanceOf()), - Token::kIS, /*argument_count=*/2, - /*checked_argument_count=*/2); - instructions += BranchIfTrue(&then_entry, &otherwise_entry, /*negate=*/false); - Fragment otherwise_instructions(otherwise_entry); - otherwise_instructions += Goto(join); + if (!IG->null_safety()) { + // Without sound null safety we need to check that the switch variable is + // not null. If it is null, we go to [join] which is either the default + // case or the exit of the switch statement. + TargetEntryInstr* null_entry; + TargetEntryInstr* non_null_entry; - instructions = Fragment(instructions.entry, then_entry); + instructions += LoadLocal(scopes()->switch_variable); + instructions += BranchIfNull(&null_entry, &non_null_entry); + + Fragment null_instructions(null_entry); + null_instructions += Goto(join); + + instructions = Fragment(instructions.entry, non_null_entry); + } if (helper->is_enum_switch()) { // For an enum switch, we need to load the enum index from the switch @@ -5224,10 +5217,7 @@ Fragment StreamingFlowGraphBuilder::BuildBinarySearchSwitch( branch_instructions += LoadLocal(scopes()->switch_variable); branch_instructions += Constant(middle_expression.integer()); branch_instructions += - InstanceCall(middle_expression.position(), - Symbols::LessEqualOperator(), Token::kLTE, - /*argument_count=*/2, - /*checked_argument_count=*/2); + B->IntRelationalOp(middle_expression.position(), Token::kLTE); branch_instructions += BranchIfTrue(&then_entry, &otherwise_entry, /*negate=*/false); @@ -5242,10 +5232,7 @@ Fragment StreamingFlowGraphBuilder::BuildBinarySearchSwitch( upper_branch_instructions += LoadLocal(scopes()->switch_variable); upper_branch_instructions += Constant(next_expression.integer()); upper_branch_instructions += - InstanceCall(next_expression.position(), - Symbols::GreaterEqualOperator(), Token::kGTE, - /*argument_count=*/2, - /*checked_argument_count=*/2); + B->IntRelationalOp(next_expression.position(), Token::kGTE); upper_branch_instructions += BranchIfTrue(&then_entry, &otherwise_entry, /*negate=*/false); @@ -5260,6 +5247,10 @@ Fragment StreamingFlowGraphBuilder::BuildBinarySearchSwitch( stack.Add( SwitchRange::Branch(range.min(), middle, lower_branch_instructions)); } + + if (current_instructions.is_empty()) { + current_instructions = branch_instructions; + } } return Fragment(current_instructions.entry, join_instructions.current); @@ -5302,9 +5293,7 @@ Fragment StreamingFlowGraphBuilder::BuildJumpTableSwitch(SwitchHelper* helper) { if (helper->RequiresLowerBoundCheck()) { current_instructions += LoadLocal(scopes()->switch_variable); current_instructions += Constant(expression_min); - current_instructions += InstanceCall(pos, Symbols::GreaterEqualOperator(), - Token::kGTE, /*argument_count=*/2, - /*checked_argument_count=*/2); + current_instructions += B->IntRelationalOp(pos, Token::kGTE); current_instructions += BranchIfTrue(&then_entry, &otherwise_entry, /*negate=*/false); Fragment otherwise_instructions(otherwise_entry); @@ -5316,9 +5305,7 @@ Fragment StreamingFlowGraphBuilder::BuildJumpTableSwitch(SwitchHelper* helper) { if (helper->RequiresUpperBoundCheck()) { current_instructions += LoadLocal(scopes()->switch_variable); current_instructions += Constant(expression_max); - current_instructions += InstanceCall(pos, Symbols::LessEqualOperator(), - Token::kLTE, /*argument_count=*/2, - /*checked_argument_count=*/2); + current_instructions += B->IntRelationalOp(pos, Token::kLTE); current_instructions += BranchIfTrue(&then_entry, &otherwise_entry, /*negate=*/false); Fragment otherwise_instructions(otherwise_entry); diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index a39ff4a685f..ca23a4bb2a9 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -4287,6 +4287,39 @@ Fragment FlowGraphBuilder::IntToBool() { return body; } +Fragment FlowGraphBuilder::IntRelationalOp(TokenPosition position, + Token::Kind kind) { + if (CompilerState::Current().is_aot()) { + Value* right = Pop(); + Value* left = Pop(); + RelationalOpInstr* instr = new (Z) RelationalOpInstr( + InstructionSource(position), kind, left, right, kMintCid, + GetNextDeoptId(), Instruction::SpeculativeMode::kNotSpeculative); + Push(instr); + return Fragment(instr); + } + const String* name = nullptr; + switch (kind) { + case Token::kLT: + name = &Symbols::LAngleBracket(); + break; + case Token::kGT: + name = &Symbols::RAngleBracket(); + break; + case Token::kLTE: + name = &Symbols::LessEqualOperator(); + break; + case Token::kGTE: + name = &Symbols::GreaterEqualOperator(); + break; + default: + UNREACHABLE(); + } + return InstanceCall( + position, *name, kind, /*type_args_len=*/0, /*argument_count=*/2, + /*argument_names=*/Array::null_array(), /*checked_argument_count=*/2); +} + Fragment FlowGraphBuilder::NativeReturn( const compiler::ffi::CallbackMarshaller& marshaller) { auto* instr = new (Z) diff --git a/runtime/vm/compiler/frontend/kernel_to_il.h b/runtime/vm/compiler/frontend/kernel_to_il.h index acb57446b1f..a4bad24a50c 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.h +++ b/runtime/vm/compiler/frontend/kernel_to_il.h @@ -298,6 +298,9 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder { // Converts 0 to false and the rest to true. Fragment IntToBool(); + // Compares arbitrary integers. + Fragment IntRelationalOp(TokenPosition position, Token::Kind kind); + // Creates an ffi.Pointer holding a given address. Fragment FfiPointerFromAddress();