[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 <vegorov@google.com>
Commit-Queue: Stephen Adams <sra@google.com>

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;
 }
 

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;

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);

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);

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) {

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

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) {

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.

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",

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) {