diff --git a/runtime/vm/compiler/backend/flow_graph.cc b/runtime/vm/compiler/backend/flow_graph.cc index 8f354f6105d..5a7b15d9f30 100644 --- a/runtime/vm/compiler/backend/flow_graph.cc +++ b/runtime/vm/compiler/backend/flow_graph.cc @@ -620,11 +620,15 @@ bool FlowGraph::ShouldOmitCheckBoundsIn(const Function& caller) { static Definition* CreateCheckBound(Zone* zone, Definition* length, Definition* index, - intptr_t deopt_id) { + intptr_t deopt_id, + bool omit_check) { Value* val1 = new (zone) Value(length); Value* val2 = new (zone) Value(index); if (CompilerState::Current().is_aot()) { - return new (zone) GenericCheckBoundInstr(val1, val2, deopt_id); + return new (zone) GenericCheckBoundInstr( + val1, val2, deopt_id, + omit_check ? GenericCheckBoundInstr::Mode::kPhantom + : GenericCheckBoundInstr::Mode::kReal); } return new (zone) CheckArrayBoundInstr(val1, val2, deopt_id); } @@ -634,10 +638,9 @@ Instruction* FlowGraph::AppendCheckBound(Instruction* cursor, Definition** index, intptr_t deopt_id, Environment* env) { - if (!ShouldOmitCheckBoundsIn(env->function())) { - *index = CreateCheckBound(zone(), length, *index, deopt_id); - cursor = AppendTo(cursor, *index, env, FlowGraph::kValue); - } + *index = CreateCheckBound(zone(), length, *index, deopt_id, + ShouldOmitCheckBoundsIn(env->function())); + cursor = AppendTo(cursor, *index, env, FlowGraph::kValue); return cursor; } diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index e36dfcd9a21..e70f3ea5646 100644 --- a/runtime/vm/compiler/backend/il.cc +++ b/runtime/vm/compiler/backend/il.cc @@ -6211,6 +6211,13 @@ void CheckClassInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ Bind(&is_ok); } +Definition* GenericCheckBoundInstr::Canonicalize(FlowGraph* flow_graph) { + if (!flow_graph->is_licm_allowed()) { + if (IsPhantom()) return index()->definition(); + } + return CheckBoundBaseInstr::Canonicalize(flow_graph); +} + LocationSummary* GenericCheckBoundInstr::MakeLocationSummary(Zone* zone, bool opt) const { const intptr_t kNumInputs = 2; diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h index d62feb64caf..2dcc427d011 100644 --- a/runtime/vm/compiler/backend/il.h +++ b/runtime/vm/compiler/backend/il.h @@ -10871,16 +10871,32 @@ class CheckArrayBoundInstr : public CheckBoundBaseInstr { // or otherwise throws an out-of-bounds exception (viz. non-speculative). class GenericCheckBoundInstr : public CheckBoundBaseInstr { public: + enum Mode { + kReal, + + // Phantom checks serve as dependencies inhibiting illegal code motion but + // are removed before code generation. Phantom checks are inserted due to + // unsafe annotations. An early-phaee path-sensitive bounds check removal + // optimization can be implemented by replacing a real check with a phantom + // check. + kPhantom + }; + // We prefer to have unboxed inputs on 64-bit where values can fit into a // register. static bool UseUnboxedRepresentation() { return compiler::target::kWordSize == 8; } - GenericCheckBoundInstr(Value* length, Value* index, intptr_t deopt_id) - : CheckBoundBaseInstr(length, index, deopt_id) {} + GenericCheckBoundInstr(Value* length, + Value* index, + intptr_t deopt_id, + Mode mode = Mode::kReal) + : CheckBoundBaseInstr(length, index, deopt_id), mode_(mode) {} - virtual bool AttributesEqual(const Instruction& other) const { return true; } + virtual bool AttributesEqual(const Instruction& other) const { + return other.AsGenericCheckBound()->mode_ == mode_; + } DECLARE_INSTRUCTION(GenericCheckBound) @@ -10902,6 +10918,8 @@ class GenericCheckBoundInstr : public CheckBoundBaseInstr { return UseUnboxedRepresentation() ? kUnboxedInt64 : kTagged; } + virtual Definition* Canonicalize(FlowGraph* flow_graph); + // GenericCheckBound can implicitly call Dart code (RangeError or // ArgumentError constructor), so it can lazily deopt. virtual bool ComputeCanDeoptimize() const { return false; } @@ -10915,7 +10933,16 @@ class GenericCheckBoundInstr : public CheckBoundBaseInstr { return SlowPathSharingSupported(is_optimizing); } - DECLARE_EMPTY_SERIALIZATION(GenericCheckBoundInstr, CheckBoundBaseInstr) + bool IsPhantom() const { return mode_ == Mode::kPhantom; } + + PRINT_OPERANDS_TO_SUPPORT + +#define FIELD_LIST(F) F(const Mode, mode_) + + DECLARE_INSTRUCTION_SERIALIZABLE_FIELDS(GenericCheckBoundInstr, + CheckBoundBaseInstr, + FIELD_LIST) +#undef FIELD_LIST private: DISALLOW_COPY_AND_ASSIGN(GenericCheckBoundInstr); diff --git a/runtime/vm/compiler/backend/il_printer.cc b/runtime/vm/compiler/backend/il_printer.cc index 08fc69919f7..306ae467607 100644 --- a/runtime/vm/compiler/backend/il_printer.cc +++ b/runtime/vm/compiler/backend/il_printer.cc @@ -1606,6 +1606,13 @@ void MoveArgumentInstr::PrintOperandsTo(BaseTextBuffer* f) const { value()->PrintTo(f); } +void GenericCheckBoundInstr::PrintOperandsTo(BaseTextBuffer* f) const { + Definition::PrintOperandsTo(f); + if (IsPhantom()) { + f->AddString(", phantom"); + } +} + void GotoInstr::PrintTo(BaseTextBuffer* f) const { if (HasParallelMove()) { parallel_move()->PrintTo(f); diff --git a/runtime/vm/compiler/backend/inliner.cc b/runtime/vm/compiler/backend/inliner.cc index ee06dde3b17..9d533028dd9 100644 --- a/runtime/vm/compiler/backend/inliner.cc +++ b/runtime/vm/compiler/backend/inliner.cc @@ -702,6 +702,14 @@ static bool IsSmallLeafOrReduction(int inlining_depth, } continue; } + if (auto check = current->AsGenericCheckBound()) { + if (check->IsPhantom()) { + // Discount the check since it is guaranteed to be removed. + instruction_count -= 1; + // TODO(dartbug.com/56902): The bound (length input) might also become + // dead. Discount these instructions too. + } + } } } if (call_count > 0) { diff --git a/runtime/vm/compiler/backend/pragma_unsafe_no_bounds_check_test.cc b/runtime/vm/compiler/backend/pragma_unsafe_no_bounds_check_test.cc new file mode 100644 index 00000000000..c2c0e5acc91 --- /dev/null +++ b/runtime/vm/compiler/backend/pragma_unsafe_no_bounds_check_test.cc @@ -0,0 +1,108 @@ +// Copyright (c) 2024, 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/compiler/backend/il_printer.h" +#include "vm/compiler/backend/il_test_helper.h" +#include "vm/compiler/compiler_pass.h" +#include "vm/object.h" +#include "vm/unit_test.h" + +namespace dart { + +static int CountCheckBounds(FlowGraph* flow_graph) { + int checks = 0; + for (BlockIterator block_it = flow_graph->reverse_postorder_iterator(); + !block_it.Done(); block_it.Advance()) { + for (ForwardInstructionIterator it(block_it.Current()); !it.Done(); + it.Advance()) { + if (it.Current()->IsCheckBoundBase()) { + checks++; + } + } + } + return checks; +} + +ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma) { + const char* kScript = R"( + import 'dart:typed_data'; + + @pragma('vm:unsafe:no-bounds-checks') + @pragma('vm:prefer-inline') + int foo(Uint8List list) { + int result = 0; + for (int i = 0; i < 10; i++) { + result = list[i]; + } + return result; + } + + int test(Uint8List list) { + return foo(list); + } + )"; + + const auto& root_library = Library::Handle(LoadTestScript(kScript)); + const auto& function = Function::Handle(GetFunction(root_library, "test")); + + TestPipeline pipeline(function, CompilerPass::kAOT); + auto flow_graph = pipeline.RunPasses({}); + EXPECT_EQ(0, CountCheckBounds(flow_graph)); +} + +ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma_learning) { + // Test that BCE takes into account (i.e. 'learns from') checks that are + // annotated to be removed. + const char* kScript = R"( + import 'dart:typed_data'; + + @pragma('vm:unsafe:no-bounds-checks') + @pragma('vm:prefer-inline') + int load(Uint8List list, int index) => list[index]; + + int test(Uint8List list) { + int value1 = load(list, 10); + int value2 = list[5]; + return value1 + value2; + } + )"; + + const auto& root_library = Library::Handle(LoadTestScript(kScript)); + const auto& function = Function::Handle(GetFunction(root_library, "test")); + + TestPipeline pipeline(function, CompilerPass::kAOT); + auto flow_graph = pipeline.RunPasses({}); + + // No checks because unsafe (trusted) check `list[10]` dominates `list[5]`. + EXPECT_EQ(0, CountCheckBounds(flow_graph)); +} + +ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma_learning_control) { + // Sister test to BoundsCheckElimination_Pragma_learning that shows without + // the annotation, there is a check that corresponds to the check removed via + // the annotation. + const char* kScript = R"( + import 'dart:typed_data'; + + @pragma('vm:prefer-inline') + int load(Uint8List list, int index) => list[index]; + + int test(Uint8List list) { + int value1 = load(list, 10); + int value2 = list[5]; + return value1 + value2; + } + )"; + + const auto& root_library = Library::Handle(LoadTestScript(kScript)); + const auto& function = Function::Handle(GetFunction(root_library, "test")); + + TestPipeline pipeline(function, CompilerPass::kAOT); + auto flow_graph = pipeline.RunPasses({}); + + // Single check because `list[10]` dominates `list[5]`. + EXPECT_EQ(1, CountCheckBounds(flow_graph)); +} + +} // namespace dart diff --git a/runtime/vm/compiler/backend/redundancy_elimination_test.cc b/runtime/vm/compiler/backend/redundancy_elimination_test.cc index 8a20cf66b26..06a3efc2543 100644 --- a/runtime/vm/compiler/backend/redundancy_elimination_test.cc +++ b/runtime/vm/compiler/backend/redundancy_elimination_test.cc @@ -1596,37 +1596,6 @@ ISOLATE_UNIT_TEST_CASE(CheckStackOverflowElimination_NoInterruptsPragma) { } } -ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma) { - const char* kScript = R"( - import 'dart:typed_data'; - - @pragma('vm:unsafe:no-bounds-checks') - @pragma('vm:prefer-inline') - int foo(Uint8List list) { - int result = 0; - for (int i = 0; i < 10; i++) { - result = list[i]; - } - return result; - } - - int test(Uint8List list) { - return foo(list); - } - )"; - - const auto& root_library = Library::Handle(LoadTestScript(kScript)); - const auto& function = Function::Handle(GetFunction(root_library, "test")); - - TestPipeline pipeline(function, CompilerPass::kAOT); - auto flow_graph = pipeline.RunPasses({}); - for (auto block : flow_graph->postorder()) { - for (auto instr : block->instructions()) { - EXPECT_PROPERTY(instr, !it.IsCheckBoundBase()); - } - } -} - // This test checks that CSE unwraps redefinitions when comparing all // instructions except loads, which are handled specially. ISOLATE_UNIT_TEST_CASE(CSE_Redefinitions) { diff --git a/runtime/vm/compiler/call_specializer.cc b/runtime/vm/compiler/call_specializer.cc index b28a319a017..dedf2c359c6 100644 --- a/runtime/vm/compiler/call_specializer.cc +++ b/runtime/vm/compiler/call_specializer.cc @@ -1607,16 +1607,17 @@ void TypedDataSpecializer::AppendMutableCheck(TemplateDartCall<0>* call, void TypedDataSpecializer::AppendBoundsCheck(TemplateDartCall<0>* call, Definition* array, Definition** index) { - if (flow_graph_->ShouldOmitCheckBoundsIn(call->env()->function())) { - return; - } + auto omit_check = + flow_graph_->ShouldOmitCheckBoundsIn(call->env()->function()); auto length = new (Z) LoadFieldInstr( new (Z) Value(array), Slot::TypedDataBase_length(), call->source()); flow_graph_->InsertBefore(call, length, call->env(), FlowGraph::kValue); auto check = new (Z) GenericCheckBoundInstr( - new (Z) Value(length), new (Z) Value(*index), DeoptId::kNone); + new (Z) Value(length), new (Z) Value(*index), DeoptId::kNone, + omit_check ? GenericCheckBoundInstr::Mode::kPhantom + : GenericCheckBoundInstr::Mode::kReal); flow_graph_->InsertBefore(call, check, call->env(), FlowGraph::kValue); // Use data dependency as control dependency. diff --git a/runtime/vm/compiler/compiler_sources.gni b/runtime/vm/compiler/compiler_sources.gni index d043b370deb..4cf854ae4ea 100644 --- a/runtime/vm/compiler/compiler_sources.gni +++ b/runtime/vm/compiler/compiler_sources.gni @@ -176,6 +176,7 @@ compiler_sources_tests = [ "backend/locations_helpers_test.cc", "backend/loops_test.cc", "backend/memory_copy_test.cc", + "backend/pragma_unsafe_no_bounds_check_test.cc", "backend/range_analysis_test.cc", "backend/reachability_fence_test.cc", "backend/redundancy_elimination_test.cc", diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc index e3778e7aba2..39097167053 100644 --- a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc +++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc @@ -448,16 +448,14 @@ Fragment BaseFlowGraphBuilder::GenericCheckBound() { // problems with JIT (even though should_omit_check_bounds() will be false // in JIT). const intptr_t deopt_id = GetNextDeoptId(); - if (should_omit_check_bounds()) { - // Drop length but preserve index. - return DropTempsPreserveTop(/*num_temps_to_drop=*/1); - } else { - Value* index = Pop(); - Value* length = Pop(); - auto* instr = new (Z) GenericCheckBoundInstr(length, index, deopt_id); - Push(instr); - return Fragment(instr); - } + Value* index = Pop(); + Value* length = Pop(); + auto* instr = new (Z) GenericCheckBoundInstr( + length, index, deopt_id, + should_omit_check_bounds() ? GenericCheckBoundInstr::Mode::kPhantom + : GenericCheckBoundInstr::Mode::kReal); + Push(instr); + return Fragment(instr); } Fragment BaseFlowGraphBuilder::LoadUntagged(intptr_t offset) {