Skip to content

Commit 066962f

Browse files
rakudramaCommit Queue
authored andcommitted
[vm] Defer removal of bounds checks
Change how `@pragma('vm:unsafe:no-bounds-checks')` works. Instead of never inserting the bounds check, the bounds check is added to the flow graph with a flag that it should later be removed, `omit_check`. The removal occurs in `RangeAnalysis::EliminateRedundantBoundsChecks`. This ensures that the indexed load is pinned by the check, preventing illegal code motion. `@pragma('vm:unsafe:no-bounds-checks')` is not a mechanism to allow unsafe access. It means that the access is known to be safe because of invariants not apparent to the compiler. Keeping the bounds check in the flow graph allows range analysis to learn from the bounds constraints and perhaps remove other checks. A test was added for this scenario. I didn't see this in the wild, but I did see one case where a refined range allowed a boxing to be removed. Bug: #56808 TEST=BoundsCheckElimination_Pragma_learning, BoundsCheckElimination_Pragma_learning_control Change-Id: I5b3f4470d6c40c988a8a0ee563c765f4f5f9128b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389620 Reviewed-by: Slava Egorov <[email protected]> Commit-Queue: Stephen Adams <[email protected]>
1 parent cec29f1 commit 066962f

File tree

10 files changed

+184
-55
lines changed

10 files changed

+184
-55
lines changed

runtime/vm/compiler/backend/flow_graph.cc

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -620,11 +620,15 @@ bool FlowGraph::ShouldOmitCheckBoundsIn(const Function& caller) {
620620
static Definition* CreateCheckBound(Zone* zone,
621621
Definition* length,
622622
Definition* index,
623-
intptr_t deopt_id) {
623+
intptr_t deopt_id,
624+
bool omit_check) {
624625
Value* val1 = new (zone) Value(length);
625626
Value* val2 = new (zone) Value(index);
626627
if (CompilerState::Current().is_aot()) {
627-
return new (zone) GenericCheckBoundInstr(val1, val2, deopt_id);
628+
return new (zone) GenericCheckBoundInstr(
629+
val1, val2, deopt_id,
630+
omit_check ? GenericCheckBoundInstr::Mode::kPhantom
631+
: GenericCheckBoundInstr::Mode::kReal);
628632
}
629633
return new (zone) CheckArrayBoundInstr(val1, val2, deopt_id);
630634
}
@@ -634,10 +638,9 @@ Instruction* FlowGraph::AppendCheckBound(Instruction* cursor,
634638
Definition** index,
635639
intptr_t deopt_id,
636640
Environment* env) {
637-
if (!ShouldOmitCheckBoundsIn(env->function())) {
638-
*index = CreateCheckBound(zone(), length, *index, deopt_id);
639-
cursor = AppendTo(cursor, *index, env, FlowGraph::kValue);
640-
}
641+
*index = CreateCheckBound(zone(), length, *index, deopt_id,
642+
ShouldOmitCheckBoundsIn(env->function()));
643+
cursor = AppendTo(cursor, *index, env, FlowGraph::kValue);
641644
return cursor;
642645
}
643646

runtime/vm/compiler/backend/il.cc

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6211,6 +6211,13 @@ void CheckClassInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
62116211
__ Bind(&is_ok);
62126212
}
62136213

6214+
Definition* GenericCheckBoundInstr::Canonicalize(FlowGraph* flow_graph) {
6215+
if (!flow_graph->is_licm_allowed()) {
6216+
if (IsPhantom()) return index()->definition();
6217+
}
6218+
return CheckBoundBaseInstr::Canonicalize(flow_graph);
6219+
}
6220+
62146221
LocationSummary* GenericCheckBoundInstr::MakeLocationSummary(Zone* zone,
62156222
bool opt) const {
62166223
const intptr_t kNumInputs = 2;

runtime/vm/compiler/backend/il.h

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10871,16 +10871,32 @@ class CheckArrayBoundInstr : public CheckBoundBaseInstr {
1087110871
// or otherwise throws an out-of-bounds exception (viz. non-speculative).
1087210872
class GenericCheckBoundInstr : public CheckBoundBaseInstr {
1087310873
public:
10874+
enum Mode {
10875+
kReal,
10876+
10877+
// Phantom checks serve as dependencies inhibiting illegal code motion but
10878+
// are removed before code generation. Phantom checks are inserted due to
10879+
// unsafe annotations. An early-phaee path-sensitive bounds check removal
10880+
// optimization can be implemented by replacing a real check with a phantom
10881+
// check.
10882+
kPhantom
10883+
};
10884+
1087410885
// We prefer to have unboxed inputs on 64-bit where values can fit into a
1087510886
// register.
1087610887
static bool UseUnboxedRepresentation() {
1087710888
return compiler::target::kWordSize == 8;
1087810889
}
1087910890

10880-
GenericCheckBoundInstr(Value* length, Value* index, intptr_t deopt_id)
10881-
: CheckBoundBaseInstr(length, index, deopt_id) {}
10891+
GenericCheckBoundInstr(Value* length,
10892+
Value* index,
10893+
intptr_t deopt_id,
10894+
Mode mode = Mode::kReal)
10895+
: CheckBoundBaseInstr(length, index, deopt_id), mode_(mode) {}
1088210896

10883-
virtual bool AttributesEqual(const Instruction& other) const { return true; }
10897+
virtual bool AttributesEqual(const Instruction& other) const {
10898+
return other.AsGenericCheckBound()->mode_ == mode_;
10899+
}
1088410900

1088510901
DECLARE_INSTRUCTION(GenericCheckBound)
1088610902

@@ -10902,6 +10918,8 @@ class GenericCheckBoundInstr : public CheckBoundBaseInstr {
1090210918
return UseUnboxedRepresentation() ? kUnboxedInt64 : kTagged;
1090310919
}
1090410920

10921+
virtual Definition* Canonicalize(FlowGraph* flow_graph);
10922+
1090510923
// GenericCheckBound can implicitly call Dart code (RangeError or
1090610924
// ArgumentError constructor), so it can lazily deopt.
1090710925
virtual bool ComputeCanDeoptimize() const { return false; }
@@ -10915,7 +10933,16 @@ class GenericCheckBoundInstr : public CheckBoundBaseInstr {
1091510933
return SlowPathSharingSupported(is_optimizing);
1091610934
}
1091710935

10918-
DECLARE_EMPTY_SERIALIZATION(GenericCheckBoundInstr, CheckBoundBaseInstr)
10936+
bool IsPhantom() const { return mode_ == Mode::kPhantom; }
10937+
10938+
PRINT_OPERANDS_TO_SUPPORT
10939+
10940+
#define FIELD_LIST(F) F(const Mode, mode_)
10941+
10942+
DECLARE_INSTRUCTION_SERIALIZABLE_FIELDS(GenericCheckBoundInstr,
10943+
CheckBoundBaseInstr,
10944+
FIELD_LIST)
10945+
#undef FIELD_LIST
1091910946

1092010947
private:
1092110948
DISALLOW_COPY_AND_ASSIGN(GenericCheckBoundInstr);

runtime/vm/compiler/backend/il_printer.cc

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1606,6 +1606,13 @@ void MoveArgumentInstr::PrintOperandsTo(BaseTextBuffer* f) const {
16061606
value()->PrintTo(f);
16071607
}
16081608

1609+
void GenericCheckBoundInstr::PrintOperandsTo(BaseTextBuffer* f) const {
1610+
Definition::PrintOperandsTo(f);
1611+
if (IsPhantom()) {
1612+
f->AddString(", phantom");
1613+
}
1614+
}
1615+
16091616
void GotoInstr::PrintTo(BaseTextBuffer* f) const {
16101617
if (HasParallelMove()) {
16111618
parallel_move()->PrintTo(f);

runtime/vm/compiler/backend/inliner.cc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,14 @@ static bool IsSmallLeafOrReduction(int inlining_depth,
702702
}
703703
continue;
704704
}
705+
if (auto check = current->AsGenericCheckBound()) {
706+
if (check->IsPhantom()) {
707+
// Discount the check since it is guaranteed to be removed.
708+
instruction_count -= 1;
709+
// TODO(dartbug.com/56902): The bound (length input) might also become
710+
// dead. Discount these instructions too.
711+
}
712+
}
705713
}
706714
}
707715
if (call_count > 0) {
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
#include "vm/compiler/backend/il_printer.h"
6+
#include "vm/compiler/backend/il_test_helper.h"
7+
#include "vm/compiler/compiler_pass.h"
8+
#include "vm/object.h"
9+
#include "vm/unit_test.h"
10+
11+
namespace dart {
12+
13+
static int CountCheckBounds(FlowGraph* flow_graph) {
14+
int checks = 0;
15+
for (BlockIterator block_it = flow_graph->reverse_postorder_iterator();
16+
!block_it.Done(); block_it.Advance()) {
17+
for (ForwardInstructionIterator it(block_it.Current()); !it.Done();
18+
it.Advance()) {
19+
if (it.Current()->IsCheckBoundBase()) {
20+
checks++;
21+
}
22+
}
23+
}
24+
return checks;
25+
}
26+
27+
ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma) {
28+
const char* kScript = R"(
29+
import 'dart:typed_data';
30+
31+
@pragma('vm:unsafe:no-bounds-checks')
32+
@pragma('vm:prefer-inline')
33+
int foo(Uint8List list) {
34+
int result = 0;
35+
for (int i = 0; i < 10; i++) {
36+
result = list[i];
37+
}
38+
return result;
39+
}
40+
41+
int test(Uint8List list) {
42+
return foo(list);
43+
}
44+
)";
45+
46+
const auto& root_library = Library::Handle(LoadTestScript(kScript));
47+
const auto& function = Function::Handle(GetFunction(root_library, "test"));
48+
49+
TestPipeline pipeline(function, CompilerPass::kAOT);
50+
auto flow_graph = pipeline.RunPasses({});
51+
EXPECT_EQ(0, CountCheckBounds(flow_graph));
52+
}
53+
54+
ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma_learning) {
55+
// Test that BCE takes into account (i.e. 'learns from') checks that are
56+
// annotated to be removed.
57+
const char* kScript = R"(
58+
import 'dart:typed_data';
59+
60+
@pragma('vm:unsafe:no-bounds-checks')
61+
@pragma('vm:prefer-inline')
62+
int load(Uint8List list, int index) => list[index];
63+
64+
int test(Uint8List list) {
65+
int value1 = load(list, 10);
66+
int value2 = list[5];
67+
return value1 + value2;
68+
}
69+
)";
70+
71+
const auto& root_library = Library::Handle(LoadTestScript(kScript));
72+
const auto& function = Function::Handle(GetFunction(root_library, "test"));
73+
74+
TestPipeline pipeline(function, CompilerPass::kAOT);
75+
auto flow_graph = pipeline.RunPasses({});
76+
77+
// No checks because unsafe (trusted) check `list[10]` dominates `list[5]`.
78+
EXPECT_EQ(0, CountCheckBounds(flow_graph));
79+
}
80+
81+
ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma_learning_control) {
82+
// Sister test to BoundsCheckElimination_Pragma_learning that shows without
83+
// the annotation, there is a check that corresponds to the check removed via
84+
// the annotation.
85+
const char* kScript = R"(
86+
import 'dart:typed_data';
87+
88+
@pragma('vm:prefer-inline')
89+
int load(Uint8List list, int index) => list[index];
90+
91+
int test(Uint8List list) {
92+
int value1 = load(list, 10);
93+
int value2 = list[5];
94+
return value1 + value2;
95+
}
96+
)";
97+
98+
const auto& root_library = Library::Handle(LoadTestScript(kScript));
99+
const auto& function = Function::Handle(GetFunction(root_library, "test"));
100+
101+
TestPipeline pipeline(function, CompilerPass::kAOT);
102+
auto flow_graph = pipeline.RunPasses({});
103+
104+
// Single check because `list[10]` dominates `list[5]`.
105+
EXPECT_EQ(1, CountCheckBounds(flow_graph));
106+
}
107+
108+
} // namespace dart

runtime/vm/compiler/backend/redundancy_elimination_test.cc

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1596,37 +1596,6 @@ ISOLATE_UNIT_TEST_CASE(CheckStackOverflowElimination_NoInterruptsPragma) {
15961596
}
15971597
}
15981598

1599-
ISOLATE_UNIT_TEST_CASE(BoundsCheckElimination_Pragma) {
1600-
const char* kScript = R"(
1601-
import 'dart:typed_data';
1602-
1603-
@pragma('vm:unsafe:no-bounds-checks')
1604-
@pragma('vm:prefer-inline')
1605-
int foo(Uint8List list) {
1606-
int result = 0;
1607-
for (int i = 0; i < 10; i++) {
1608-
result = list[i];
1609-
}
1610-
return result;
1611-
}
1612-
1613-
int test(Uint8List list) {
1614-
return foo(list);
1615-
}
1616-
)";
1617-
1618-
const auto& root_library = Library::Handle(LoadTestScript(kScript));
1619-
const auto& function = Function::Handle(GetFunction(root_library, "test"));
1620-
1621-
TestPipeline pipeline(function, CompilerPass::kAOT);
1622-
auto flow_graph = pipeline.RunPasses({});
1623-
for (auto block : flow_graph->postorder()) {
1624-
for (auto instr : block->instructions()) {
1625-
EXPECT_PROPERTY(instr, !it.IsCheckBoundBase());
1626-
}
1627-
}
1628-
}
1629-
16301599
// This test checks that CSE unwraps redefinitions when comparing all
16311600
// instructions except loads, which are handled specially.
16321601
ISOLATE_UNIT_TEST_CASE(CSE_Redefinitions) {

runtime/vm/compiler/call_specializer.cc

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1607,16 +1607,17 @@ void TypedDataSpecializer::AppendMutableCheck(TemplateDartCall<0>* call,
16071607
void TypedDataSpecializer::AppendBoundsCheck(TemplateDartCall<0>* call,
16081608
Definition* array,
16091609
Definition** index) {
1610-
if (flow_graph_->ShouldOmitCheckBoundsIn(call->env()->function())) {
1611-
return;
1612-
}
1610+
auto omit_check =
1611+
flow_graph_->ShouldOmitCheckBoundsIn(call->env()->function());
16131612

16141613
auto length = new (Z) LoadFieldInstr(
16151614
new (Z) Value(array), Slot::TypedDataBase_length(), call->source());
16161615
flow_graph_->InsertBefore(call, length, call->env(), FlowGraph::kValue);
16171616

16181617
auto check = new (Z) GenericCheckBoundInstr(
1619-
new (Z) Value(length), new (Z) Value(*index), DeoptId::kNone);
1618+
new (Z) Value(length), new (Z) Value(*index), DeoptId::kNone,
1619+
omit_check ? GenericCheckBoundInstr::Mode::kPhantom
1620+
: GenericCheckBoundInstr::Mode::kReal);
16201621
flow_graph_->InsertBefore(call, check, call->env(), FlowGraph::kValue);
16211622

16221623
// Use data dependency as control dependency.

runtime/vm/compiler/compiler_sources.gni

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ compiler_sources_tests = [
176176
"backend/locations_helpers_test.cc",
177177
"backend/loops_test.cc",
178178
"backend/memory_copy_test.cc",
179+
"backend/pragma_unsafe_no_bounds_check_test.cc",
179180
"backend/range_analysis_test.cc",
180181
"backend/reachability_fence_test.cc",
181182
"backend/redundancy_elimination_test.cc",

runtime/vm/compiler/frontend/base_flow_graph_builder.cc

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -448,16 +448,14 @@ Fragment BaseFlowGraphBuilder::GenericCheckBound() {
448448
// problems with JIT (even though should_omit_check_bounds() will be false
449449
// in JIT).
450450
const intptr_t deopt_id = GetNextDeoptId();
451-
if (should_omit_check_bounds()) {
452-
// Drop length but preserve index.
453-
return DropTempsPreserveTop(/*num_temps_to_drop=*/1);
454-
} else {
455-
Value* index = Pop();
456-
Value* length = Pop();
457-
auto* instr = new (Z) GenericCheckBoundInstr(length, index, deopt_id);
458-
Push(instr);
459-
return Fragment(instr);
460-
}
451+
Value* index = Pop();
452+
Value* length = Pop();
453+
auto* instr = new (Z) GenericCheckBoundInstr(
454+
length, index, deopt_id,
455+
should_omit_check_bounds() ? GenericCheckBoundInstr::Mode::kPhantom
456+
: GenericCheckBoundInstr::Mode::kReal);
457+
Push(instr);
458+
return Fragment(instr);
461459
}
462460

463461
Fragment BaseFlowGraphBuilder::LoadUntagged(intptr_t offset) {

0 commit comments

Comments
 (0)