[vm/compiler] Clean up typed data stores

mraleph · Commit Queue · commit 6249b7ed3402 · 2023-11-29T09:26:14.000Z
Implementation of stores and their inlinings had some stuff left over their since Dart 1: * No need to insert null-checks (in sound null-safety mode). * Since Dart 2 there is no need to insert (speculative) cid checks. Inputs are guaranteed to be a value of a supported implementation type. Inserting narrow speculative checks for Smis is actually leads to worse code in JIT. * There is no need to convert incomming integer values to smaller representation - the store will take care of it. This was left over from Dart 1 times when incomming integer could be _Bigint. TEST=existing tests Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-simarm_x64-try,vm-aot-linux-release-arm64-try,vm-aot-linux-release-x64-try,vm-aot-mac-product-arm64-try,vm-aot-linux-product-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-simarm_x64-try,vm-aot-linux-debug-simriscv64-try,vm-aot-android-release-arm_x64-try,vm-aot-android-release-arm64c-try Change-Id: I72cdaaecc524f1dccc63825df4f7b71241ab47a0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338600 Commit-Queue: Slava Egorov <vegorov@google.com> Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/compiler/backend/inliner.cc b/runtime/vm/compiler/backend/inliner.cc
@@ -2911,7 +2911,6 @@ static bool InlineSetIndexed(FlowGraph* flow_graph,
                              Instruction* call,
                              Definition* receiver,
                              const InstructionSource& source,
-                             const Cids* value_check,
                              FlowGraphInliner::ExactnessInfo* exactness,
                              GraphEntryInstr* graph_entry,
                              FunctionEntryInstr** entry,
@@ -3050,16 +3049,6 @@ static bool InlineSetIndexed(FlowGraph* flow_graph,
       array_cid == kExternalTypedDataUint8ArrayCid ||
       array_cid == kExternalTypedDataUint8ClampedArrayCid;
 
-  if (value_check != nullptr) {
-    // No store barrier needed because checked value is a smi, an unboxed mint,
-    // an unboxed double, an unboxed Float32x4, or unboxed Int32x4.
-    needs_store_barrier = kNoStoreBarrier;
-    Instruction* check = flow_graph->CreateCheckClass(
-        stored_value, *value_check, call->deopt_id(), call->source());
-    cursor =
-        flow_graph->AppendTo(cursor, check, call->env(), FlowGraph::kEffect);
-  }
-
   if (array_cid == kTypedDataFloat32ArrayCid) {
     stored_value = new (Z)
         DoubleToFloatInstr(new (Z) Value(stored_value), call->deopt_id());
@@ -3339,110 +3328,16 @@ static bool InlineByteArrayBaseStore(FlowGraph* flow_graph,
   (*entry)->InheritDeoptTarget(Z, call);
   Instruction* cursor = *entry;
 
-  // Prepare additional checks. In AOT Dart2, we use an explicit null check and
-  // non-speculative unboxing for most value types.
-  Cids* value_check = nullptr;
-  bool needs_null_check = false;
-  switch (view_cid) {
-    case kTypedDataInt8ArrayCid:
-    case kTypedDataUint8ArrayCid:
-    case kTypedDataUint8ClampedArrayCid:
-    case kExternalTypedDataUint8ArrayCid:
-    case kExternalTypedDataUint8ClampedArrayCid:
-    case kTypedDataInt16ArrayCid:
-    case kTypedDataUint16ArrayCid: {
-      if (CompilerState::Current().is_aot()) {
-        needs_null_check = true;
-      } else {
-        // Check that value is always smi.
-        value_check = Cids::CreateMonomorphic(Z, kSmiCid);
-      }
-      break;
-    }
-    case kTypedDataInt32ArrayCid:
-    case kTypedDataUint32ArrayCid:
-      if (CompilerState::Current().is_aot()) {
-        needs_null_check = true;
-      } else {
-        // On 64-bit platforms assume that stored value is always a smi.
-        if (compiler::target::kSmiBits >= 32) {
-          value_check = Cids::CreateMonomorphic(Z, kSmiCid);
-        }
-      }
-      break;
-    case kTypedDataFloat32ArrayCid:
-    case kTypedDataFloat64ArrayCid: {
-      // Check that value is always double.
-      if (CompilerState::Current().is_aot()) {
-        needs_null_check = true;
-      } else {
-        value_check = Cids::CreateMonomorphic(Z, kDoubleCid);
-      }
-      break;
-    }
-    case kTypedDataInt32x4ArrayCid: {
-      // Check that value is always Int32x4.
-      value_check = Cids::CreateMonomorphic(Z, kInt32x4Cid);
-      break;
-    }
-    case kTypedDataFloat32x4ArrayCid: {
-      // Check that value is always Float32x4.
-      value_check = Cids::CreateMonomorphic(Z, kFloat32x4Cid);
-      break;
-    }
-    case kTypedDataFloat64x2ArrayCid: {
-      // Check that value is always Float64x2.
-      value_check = Cids::CreateMonomorphic(Z, kFloat64x2Cid);
-      break;
-    }
-    case kTypedDataInt64ArrayCid:
-    case kTypedDataUint64ArrayCid:
-      // StoreIndexedInstr takes unboxed int64, so value is
-      // checked when unboxing. In AOT, we use an
-      // explicit null check and non-speculative unboxing.
-      needs_null_check = CompilerState::Current().is_aot();
-      break;
-    default:
-      // Array cids are already checked in the caller.
-      UNREACHABLE();
-  }
-
   Definition* stored_value = call->ArgumentAt(2);
 
-  // Handle value check.
-  if (value_check != nullptr) {
-    Instruction* check = flow_graph->CreateCheckClass(
-        stored_value, *value_check, call->deopt_id(), call->source());
-    cursor =
-        flow_graph->AppendTo(cursor, check, call->env(), FlowGraph::kEffect);
-  }
-
-  // Handle null check.
-  if (needs_null_check) {
+  // We know that the incomming type matches, but we still need to handle the
+  // null check.
+  if (!dart::Thread::Current()->isolate_group()->null_safety()) {
     String& name = String::ZoneHandle(Z, target.name());
     Instruction* check = new (Z) CheckNullInstr(
         new (Z) Value(stored_value), name, call->deopt_id(), call->source());
     cursor =
         flow_graph->AppendTo(cursor, check, call->env(), FlowGraph::kEffect);
-    // With an explicit null check, a non-speculative unbox suffices.
-    switch (view_cid) {
-      case kTypedDataFloat32ArrayCid:
-      case kTypedDataFloat64ArrayCid:
-        stored_value =
-            UnboxInstr::Create(kUnboxedDouble, new (Z) Value(stored_value),
-                               call->deopt_id(), Instruction::kNotSpeculative);
-        cursor = flow_graph->AppendTo(cursor, stored_value, call->env(),
-                                      FlowGraph::kValue);
-        break;
-      case kTypedDataInt64ArrayCid:
-      case kTypedDataUint64ArrayCid:
-        stored_value = new (Z)
-            UnboxInt64Instr(new (Z) Value(stored_value), call->deopt_id(),
-                            Instruction::kNotSpeculative);
-        cursor = flow_graph->AppendTo(cursor, stored_value, call->env(),
-                                      FlowGraph::kValue);
-        break;
-    }
   }
 
   // Handle conversions and special unboxing (to ensure unboxing instructions
@@ -3464,13 +3359,33 @@ static bool InlineByteArrayBaseStore(FlowGraph* flow_graph,
                                     FlowGraph::kValue);
       break;
     }
-    case kTypedDataFloat32ArrayCid: {
-      stored_value = new (Z)
-          DoubleToFloatInstr(new (Z) Value(stored_value), call->deopt_id());
-      cursor = flow_graph->AppendTo(cursor, stored_value, nullptr,
+
+    case kTypedDataInt64ArrayCid:
+    case kTypedDataUint64ArrayCid: {
+      stored_value =
+          new (Z) UnboxInt64Instr(new (Z) Value(stored_value), call->deopt_id(),
+                                  Instruction::kNotSpeculative);
+      cursor = flow_graph->AppendTo(cursor, stored_value, call->env(),
                                     FlowGraph::kValue);
       break;
     }
+
+    case kTypedDataFloat32ArrayCid:
+    case kTypedDataFloat64ArrayCid: {
+      stored_value =
+          UnboxInstr::Create(kUnboxedDouble, new (Z) Value(stored_value),
+                             call->deopt_id(), Instruction::kNotSpeculative);
+      cursor = flow_graph->AppendTo(cursor, stored_value, call->env(),
+                                    FlowGraph::kValue);
+      if (view_cid == kTypedDataFloat32ArrayCid) {
+        stored_value = new (Z)
+            DoubleToFloatInstr(new (Z) Value(stored_value), call->deopt_id());
+        cursor = flow_graph->AppendTo(cursor, stored_value, call->env(),
+                                      FlowGraph::kValue);
+      }
+      break;
+    }
+
     case kTypedDataInt32ArrayCid: {
       stored_value = new (Z)
           UnboxInt32Instr(UnboxInt32Instr::kTruncate,
@@ -4729,72 +4644,50 @@ bool FlowGraphInliner::TryInlineRecognizedMethod(
   }
 
   switch (kind) {
+    case MethodRecognizer::kUint8ClampedArraySetIndexed:
+    case MethodRecognizer::kExternalUint8ClampedArraySetIndexed:
+      // These require clamping. Just inline normal body instead which
+      // contains necessary clamping code.
+      return false;
+
     // Recognized []= operators.
     case MethodRecognizer::kObjectArraySetIndexed:
     case MethodRecognizer::kGrowableArraySetIndexed:
     case MethodRecognizer::kObjectArraySetIndexedUnchecked:
     case MethodRecognizer::kGrowableArraySetIndexedUnchecked:
-      return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              /* value_check = */ nullptr, exactness,
-                              graph_entry, entry, last, result);
     case MethodRecognizer::kInt8ArraySetIndexed:
     case MethodRecognizer::kUint8ArraySetIndexed:
-    case MethodRecognizer::kUint8ClampedArraySetIndexed:
     case MethodRecognizer::kExternalUint8ArraySetIndexed:
-    case MethodRecognizer::kExternalUint8ClampedArraySetIndexed:
     case MethodRecognizer::kInt16ArraySetIndexed:
-    case MethodRecognizer::kUint16ArraySetIndexed: {
-      // Optimistically assume Smi.
-      if (ic_data != nullptr &&
-          ic_data->HasDeoptReason(ICData::kDeoptCheckSmi)) {
-        // Optimistic assumption failed at least once.
-        return false;
-      }
-      Cids* value_check = Cids::CreateMonomorphic(Z, kSmiCid);
-      return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              value_check, exactness, graph_entry, entry, last,
-                              result);
-    }
+    case MethodRecognizer::kUint16ArraySetIndexed:
     case MethodRecognizer::kInt32ArraySetIndexed:
-    case MethodRecognizer::kUint32ArraySetIndexed: {
-      // Value check not needed for Int32 and Uint32 arrays because they
-      // implicitly contain unboxing instructions which check for right type.
-      return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              /* value_check = */ nullptr, exactness,
-                              graph_entry, entry, last, result);
-    }
+    case MethodRecognizer::kUint32ArraySetIndexed:
     case MethodRecognizer::kInt64ArraySetIndexed:
     case MethodRecognizer::kUint64ArraySetIndexed:
       return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              /* value_check = */ nullptr, exactness,
-                              graph_entry, entry, last, result);
+                              exactness, graph_entry, entry, last, result);
+
     case MethodRecognizer::kFloat32ArraySetIndexed:
     case MethodRecognizer::kFloat64ArraySetIndexed: {
       if (!CanUnboxDouble()) {
         return false;
       }
-      Cids* value_check = Cids::CreateMonomorphic(Z, kDoubleCid);
       return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              value_check, exactness, graph_entry, entry, last,
-                              result);
+                              exactness, graph_entry, entry, last, result);
     }
     case MethodRecognizer::kFloat32x4ArraySetIndexed: {
       if (!ShouldInlineSimd()) {
         return false;
       }
-      Cids* value_check = Cids::CreateMonomorphic(Z, kFloat32x4Cid);
       return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              value_check, exactness, graph_entry, entry, last,
-                              result);
+                              exactness, graph_entry, entry, last, result);
     }
     case MethodRecognizer::kFloat64x2ArraySetIndexed: {
       if (!ShouldInlineSimd()) {
         return false;
       }
-      Cids* value_check = Cids::CreateMonomorphic(Z, kFloat64x2Cid);
       return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
-                              value_check, exactness, graph_entry, entry, last,
-                              result);
+                              exactness, graph_entry, entry, last, result);
     }
     case MethodRecognizer::kByteArrayBaseSetInt8:
       return InlineByteArrayBaseStore(flow_graph, target, call, receiver,
diff --git a/runtime/vm/compiler/recognized_methods_list.h b/runtime/vm/compiler/recognized_methods_list.h
@@ -119,7 +119,6 @@ namespace dart {
   V(_TypedListBase, _memMove4, TypedData_memMove4, 0xcfe37726)                 \
   V(_TypedListBase, _memMove8, TypedData_memMove8, 0xd1d8e325)                 \
   V(_TypedListBase, _memMove16, TypedData_memMove16, 0x07861cd5)               \
-  V(::, _toClampedUint8, ConvertIntToClampedUint8, 0xd0e522d0)                 \
   V(::, _typedDataIndexCheck, TypedDataIndexCheck, 0xc54f594f)                 \
   V(::, _byteDataByteOffsetCheck, ByteDataByteOffsetCheck, 0x4ae73104)         \
   V(::, copyRangeFromUint8ListToOneByteString,                                 \
@@ -393,25 +392,25 @@ namespace dart {
 
 #define GRAPH_TYPED_DATA_INTRINSICS_LIST(V)                                    \
   V(_Int8List, [], Int8ArrayGetIndexed, 0x7b31eba4)                            \
-  V(_Int8List, []=, Int8ArraySetIndexed, 0x02734e41)                           \
+  V(_Int8List, []=, Int8ArraySetIndexed, 0x02f7bc29)                           \
   V(_Uint8List, [], Uint8ArrayGetIndexed, 0xe0aa6f64)                          \
-  V(_Uint8List, []=, Uint8ArraySetIndexed, 0x4f025a05)                         \
+  V(_Uint8List, []=, Uint8ArraySetIndexed, 0xc8fdea5d)                         \
   V(_ExternalUint8Array, [], ExternalUint8ArrayGetIndexed, 0xe0aa6f64)         \
-  V(_ExternalUint8Array, []=, ExternalUint8ArraySetIndexed, 0x4f025a05)        \
+  V(_ExternalUint8Array, []=, ExternalUint8ArraySetIndexed, 0xc8fdea5d)        \
   V(_Uint8ClampedList, [], Uint8ClampedArrayGetIndexed, 0xe0aa6f64)            \
   V(_Uint8ClampedList, []=, Uint8ClampedArraySetIndexed, 0x45020fa5)           \
   V(_ExternalUint8ClampedArray, [], ExternalUint8ClampedArrayGetIndexed,       \
     0xe0aa6f64)                                                                \
   V(_ExternalUint8ClampedArray, []=, ExternalUint8ClampedArraySetIndexed,      \
     0x45020fa5)                                                                \
   V(_Int16List, [], Int16ArrayGetIndexed, 0x5b304b04)                          \
-  V(_Int16List, []=, Int16ArraySetIndexed, 0x92f311cc)                         \
+  V(_Int16List, []=, Int16ArraySetIndexed, 0x90aeedef)                         \
   V(_Uint16List, [], Uint16ArrayGetIndexed, 0x3f092704)                        \
-  V(_Uint16List, []=, Uint16ArraySetIndexed, 0x7b35b943)                       \
+  V(_Uint16List, []=, Uint16ArraySetIndexed, 0x03eecf75)                       \
   V(_Int32List, [], Int32ArrayGetIndexed, 0x5ccdf0a3)                          \
-  V(_Int32List, []=, Int32ArraySetIndexed, 0x71278e6b)                         \
+  V(_Int32List, []=, Int32ArraySetIndexed, 0x7fb5bb97)                         \
   V(_Uint32List, [], Uint32ArrayGetIndexed, 0xac205643)                        \
-  V(_Uint32List, []=, Uint32ArraySetIndexed, 0x5ccb02eb)                       \
+  V(_Uint32List, []=, Uint32ArraySetIndexed, 0xc8d82563)                       \
   V(_Int64List, [], Int64ArrayGetIndexed, 0x95964ae3)                          \
   V(_Int64List, []=, Int64ArraySetIndexed, 0xf550bf5d)                         \
   V(_Uint64List, [], Uint64ArrayGetIndexed, 0xd25ab063)                        \
diff --git a/sdk/lib/_internal/vm/lib/typed_data_patch.dart b/sdk/lib/_internal/vm/lib/typed_data_patch.dart
