Reland "[vm/compiler] Change MemoryCopy to also take untagged addresses."

This is a reland of commit 06d7a23

This version fixes an issue when a phi node has multiple inputs with
different unboxed integer representations. The original CL made a
change where only the representations were considered, not the range
of values for the Phi calculated by range analysis. The reland goes
back to the old behavior for this case.

Also fixes the new tests on 32-bit architectures.

Original change's description:
> [vm/compiler] Change MemoryCopy to also take untagged addresses.
>
> This CL adds the ability to pass the payload address of the source
> and destination directly to the MemoryCopy instruction as an untagged
> value.
>
> The new translation of the _TypedListBase._memMoveN methods use the new
> MemoryCopy constructor, retrieving the untagged value of the data field
> of both the source and destination. This way, if inlining exposes the
> allocation of the object from which the data field is being retrieved,
> then allocation sinking can remove the intermediate allocation if there
> are no escaping uses of the object.
>
> Since Pointer.asTypedList allocates such ExternalTypedData objects,
> this CL makes that method inlined if at all possible, which removes
> the intermediate allocation if the only use of the TypedData object
> is to call setRange for memory copying purposes.
>
> This CL also separates unboxed native slots into two groups: those
> that contain untagged addresses and those that do not. The former
> group now have the kUntagged representation, which mimics the old
> use of LoadUntagged for the PointerBase data field and also ensures
> that any arithmetic operations on untagged addresses must first be
> explicitly converted to an unboxed integer and then explicitly converted
> back to untagged before being stored in a slot that contains untagged
> addresses.
>
> When a unboxed native slot that contains untagged addresses is defined,
> the definition also includes a boolean which represents whether
> addresses that may be moved by the GC can be stored in this slot or not.
> The redundancy eliminator uses this to decide whether it is safe to
> eliminate a duplicate load, replace a load with the value originally
> stored in the slot, or lift a load out of a loop.
>
> In particular, the PointerBase data field may contain GC-moveable
> addresses, but only for internal TypedData objects and views, not
> for external TypedData objects or Pointers. To allow load optimizations
> involving the latter, the LoadField and StoreField instructions now
> take boolean flags for whether loads or stores from the slot are
> guaranteed to not be GC-moveable, to override the information from
> the slot argument.
>
> Notable benchmark changes on x64 (similar for other archs unless noted):
>
> JIT:
> * FfiMemory.PointerPointer: 250.7%
> * FfiStructCopy.Copy1Bytes: -26.73% (only x64)
> * FfiStructCopy.Copy32Bytes: -25.18% (only x64)
> * MemoryCopy.64.setRange.Pointer.Uint8: 19.36%
> * MemoryCopy.64.setRange.Pointer.Double: 18.96%
> * MemoryCopy.8.setRange.Pointer.Double: 17.59%
> * MemoryCopy.8.setRange.Pointer.Uint8: 19.46%
>
> AOT:
> * FfiMemory.PointerPointer: 323.5%
> * FfiStruct.FieldLoadStore: 483.3%
> * FileIO_readwrite_64kb: 15.39%
> * FileIO_readwrite_512kb (Intel Xeon): 46.22%
> * MemoryCopy.512.setRange.Pointer.Uint8: 35.20%
> * MemoryCopy.64.setRange.Pointer.Uint8: 55.40%
> * MemoryCopy.512.setRange.Pointer.Double: 29.45%
> * MemoryCopy.64.setRange.Pointer.Double: 60.37%
> * MemoryCopy.8.setRange.Pointer.Double: 59.54%
> * MemoryCopy.8.setRange.Pointer.Uint8: 55.40%
> * FfiStructCopy.Copy32Bytes: 398.3%
> * FfiStructCopy.Copy1Bytes: 1233%
>
> TEST=vm/dart/address_local_pointer, vm/dart/pointer_as_typed_list
>
> Issue: #42072
> Fixes: #53124
>
> Cq-Include-Trybots: luci.dart.try:vm-ffi-qemu-linux-release-arm-try,vm-eager-optimization-linux-release-x64-try,vm-linux-release-x64-try,vm-linux-debug-x64-try,vm-aot-linux-release-x64-try,vm-aot-linux-debug-x64-try
> Change-Id: I563e0bfac5b1ac6cf1111649934067c12891b631
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324820
> Reviewed-by: Alexander Markov <[email protected]>
> Commit-Queue: Tess Strickland <[email protected]>
> Reviewed-by: Martin Kustermann <[email protected]>

TEST=vm/dart/address_local_pointer, vm/dart/pointer_as_typed_list

Issue: #42072
Fixes: #53124

Change-Id: Iabb0e910f12636d0ff51e711c8c9c98ad40e5811
Cq-Include-Trybots: luci.dart.try:vm-ffi-qemu-linux-release-arm-try,vm-eager-optimization-linux-release-x64-try,vm-linux-release-x64-try,vm-linux-debug-x64-try,vm-aot-linux-release-x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-release-simarm_x64-try,vm-aot-linux-debug-simarm_x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/330600
Reviewed-by: Alexander Markov <[email protected]>
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: sstrickl

pkg/vm/lib/testing/il_matchers.dart

@@ -183,6 +183,10 @@ class Env {
   void bind(String name, Map<String, dynamic> instrOrBlock) {
     final id = instrOrBlock['v'] ?? instrOrBlock['b'];
 
+    if (id == null) {
+      throw 'Instruction is not a definition or a block: ${instrOrBlock['o']}';
+    }
+
     if (nameToId.containsKey(name)) {
       if (nameToId[name] != id) {
         throw 'Binding mismatch for $name: got ${nameToId[name]} and $id';
@@ -561,6 +565,14 @@ final dynamic match = Matchers();
 /// set this field.
 late String Function(Symbol) getName;
 
+final bool isSimulator = (() {
+  final configuration = Platform.environment['DART_CONFIGURATION'];
+  if (configuration == null) {
+    throw 'Expected DART_CONFIGURATION to be defined';
+  }
+  return configuration.contains('SIM');
+})();
+
 final bool is32BitConfiguration = (() {
   final configuration = Platform.environment['DART_CONFIGURATION'];
   if (configuration == null) {

runtime/lib/typed_data.cc

@@ -29,24 +29,6 @@ static void RangeCheck(intptr_t offset_in_bytes,
   }
 }
 
-static void AlignmentCheck(intptr_t offset_in_bytes, intptr_t element_size) {
-  if ((offset_in_bytes % element_size) != 0) {
-    const auto& error = String::Handle(String::NewFormatted(
-        "Offset in bytes (%" Pd ") must be a multiple of %" Pd "",
-        offset_in_bytes, element_size));
-    Exceptions::ThrowArgumentError(error);
-  }
-}
-
-// Checks to see if a length will not result in an OOM error.
-static void LengthCheck(intptr_t len, intptr_t max) {
-  if (len < 0 || len > max) {
-    const String& error = String::Handle(String::NewFormatted(
-        "Length (%" Pd ") of object must be in range [0..%" Pd "]", len, max));
-    Exceptions::ThrowArgumentError(error);
-  }
-}
-
 DEFINE_NATIVE_ENTRY(TypedDataBase_length, 0, 1) {
   GET_NON_NULL_NATIVE_ARGUMENT(TypedDataBase, array, arguments->NativeArgAt(0));
   return Smi::New(array.Length());
@@ -148,67 +130,6 @@ DEFINE_NATIVE_ENTRY(TypedDataBase_setClampedRange, 0, 5) {
   return Object::null();
 }
 
-// Native methods for typed data allocation are recognized and implemented
-// in FlowGraphBuilder::BuildGraphOfRecognizedMethod.
-// These bodies exist only to assert that they are not used.
-#define TYPED_DATA_NEW(name)                                                   \
-  DEFINE_NATIVE_ENTRY(TypedData_##name##_new, 0, 2) {                          \
-    UNREACHABLE();                                                             \
-    return Object::null();                                                     \
-  }
-
-#define TYPED_DATA_NEW_NATIVE(name) TYPED_DATA_NEW(name)
-
-CLASS_LIST_TYPED_DATA(TYPED_DATA_NEW_NATIVE)
-#undef TYPED_DATA_NEW_NATIVE
-#undef TYPED_DATA_NEW
-
-// We check the length parameter against a possible maximum length for the
-// array based on available physical addressable memory on the system.
-//
-// More specifically
-//
-//   TypedData::MaxElements(cid) is equal to (kSmiMax / ElementSizeInBytes(cid))
-//
-// which ensures that the number of bytes the array holds is guaranteed to fit
-// into a _Smi.
-//
-// Argument 0 is type arguments and is ignored.
-static InstancePtr NewTypedDataView(intptr_t cid,
-                                    intptr_t element_size,
-                                    Zone* zone,
-                                    NativeArguments* arguments) {
-  GET_NON_NULL_NATIVE_ARGUMENT(TypedDataBase, typed_data,
-                               arguments->NativeArgAt(1));
-  GET_NON_NULL_NATIVE_ARGUMENT(Smi, offset, arguments->NativeArgAt(2));
-  GET_NON_NULL_NATIVE_ARGUMENT(Smi, len, arguments->NativeArgAt(3));
-  const intptr_t backing_length = typed_data.LengthInBytes();
-  const intptr_t offset_in_bytes = offset.Value();
-  const intptr_t length = len.Value();
-  AlignmentCheck(offset_in_bytes, element_size);
-  LengthCheck(offset_in_bytes + length * element_size, backing_length);
-  return TypedDataView::New(cid, typed_data, offset_in_bytes, length);
-}
-
-#define TYPED_DATA_VIEW_NEW(native_name, cid)                                  \
-  DEFINE_NATIVE_ENTRY(native_name, 0, 4) {                                     \
-    return NewTypedDataView(cid, TypedDataBase::ElementSizeInBytes(cid), zone, \
-                            arguments);                                        \
-  }
-
-#define TYPED_DATA_NEW_NATIVE(name)                                            \
-  TYPED_DATA_VIEW_NEW(TypedDataView_##name##View_new,                          \
-                      kTypedData##name##ViewCid)                               \
-  TYPED_DATA_VIEW_NEW(TypedDataView_Unmodifiable##name##View_new,              \
-                      kUnmodifiableTypedData##name##ViewCid)
-
-CLASS_LIST_TYPED_DATA(TYPED_DATA_NEW_NATIVE)
-TYPED_DATA_VIEW_NEW(TypedDataView_ByteDataView_new, kByteDataViewCid)
-TYPED_DATA_VIEW_NEW(TypedDataView_UnmodifiableByteDataView_new,
-                    kUnmodifiableByteDataViewCid)
-#undef TYPED_DATA_NEW_NATIVE
-#undef TYPED_DATA_VIEW_NEW
-
 #define TYPED_DATA_GETTER(getter, object, ctor, access_size)                   \
   DEFINE_NATIVE_ENTRY(TypedData_##getter, 0, 2) {                              \
     GET_NON_NULL_NATIVE_ARGUMENT(TypedDataBase, array,                         \

runtime/tests/vm/dart/address_local_pointer_il_test.dart

@@ -0,0 +1,51 @@
+// Copyright (c) 2023, 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.
+
+// Verify that returning the address of a locally created Pointer that doesn't
+// escape just returns the address used to create the Pointer without actually
+// creating it. (See https://github.com/dart-lang/sdk/issues/53124.)
+
+import 'dart:ffi';
+
+import 'package:expect/expect.dart';
+import 'package:ffi/ffi.dart';
+import 'package:vm/testing/il_matchers.dart';
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+int identity(int address) => Pointer<Void>.fromAddress(address).address;
+
+void matchIL$identity(FlowGraph graph) {
+  graph.dump();
+  if (is32BitConfiguration) {
+    // The Dart int address is truncated before being returned.
+    graph.match([
+      match.block('Graph'),
+      match.block('Function', [
+        'address' << match.Parameter(index: 0),
+        'int32' <<
+            match.IntConverter('address',
+                from: 'int64', to: 'int32', is_truncating: true),
+        'uint32' <<
+            match.IntConverter('int32',
+                from: 'int32', to: 'uint32', is_truncating: true),
+        'retval' << match.IntConverter('uint32', from: 'uint32', to: 'int64'),
+        match.Return('retval'),
+      ]),
+    ]);
+  } else {
+    graph.match([
+      match.block('Graph'),
+      match.block('Function', [
+        'address' << match.Parameter(index: 0),
+        match.Return('address'),
+      ]),
+    ]);
+  }
+}
+
+void main(List<String> args) {
+  final n = args.isEmpty ? 100 : int.parse(args.first);
+  Expect.equals(n, identity(n));
+}

runtime/tests/vm/dart/pointer_as_typed_list_il_test.dart

@@ -0,0 +1,73 @@
+// Copyright (c) 2023, 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.
+
+// Verify that we don't generate intermediate external TypedData views when
+// using setRange to copy between Pointers.
+
+import 'dart:ffi';
+
+import 'package:expect/expect.dart';
+import 'package:ffi/ffi.dart';
+import 'package:vm/testing/il_matchers.dart';
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+void copyPointerContents(Pointer<Uint8>? dest, Pointer<Uint8>? src, int n) {
+  if (dest == null || src == null) return;
+  dest.asTypedList(n).setRange(0, n, src.asTypedList(n));
+}
+
+void matchIL$copyPointerContents(FlowGraph graph) {
+  graph.dump();
+  // Since we only call it with n == 100, the third argument will get optimized
+  // away. The element_size starts as 1, but canonicalization will turn it into
+  // 4, the length to 100 / 4 == 25, and the starting offsets to 0 / 4 == 0.
+  //
+  // We could change the definition of n in main to:
+  //
+  //  final n = args.isEmpty ? 100 : int.parse(args.first);
+  //
+  // but then we'd have to wade through the generated bounds checks here.
+  graph.match([
+    match.block('Graph', [
+      'cnull' << match.Constant(value: null),
+      'c0' << match.Constant(value: 0),
+      'c25' << match.Constant(value: 25),
+    ]),
+    match.block('Function', [
+      'dest' << match.Parameter(index: 0),
+      'src' << match.Parameter(index: 1),
+      match.Branch(match.StrictCompare('dest', 'cnull'),
+          ifTrue: 'B4', ifFalse: 'B96'),
+    ]),
+    'B4' <<
+        match.block('Target', [
+          match.Return('cnull'),
+        ]),
+    'B96' <<
+        match.block('Target', [
+          'dest.data' << match.LoadField('dest', slot: 'PointerBase.data'),
+          'src.data' << match.LoadField('src', slot: 'PointerBase.data'),
+          match.MemoryCopy('src.data', 'dest.data', 'c0', 'c0', 'c25',
+              element_size: 4),
+          match.Return('cnull'),
+        ]),
+  ]);
+}
+
+void main(List<String> args) {
+  final n = 100;
+  if (isSimulator) {
+    // malloc/calloc aren't defined for simulators, so pass null instead.
+    copyPointerContents(null, null, n);
+  } else {
+    final src = malloc<Uint8>(n);
+    for (int i = 0; i < n; i++) {
+      src[i] = n - i;
+    }
+    final dest = calloc<Uint8>(n);
+    copyPointerContents(dest, src, n);
+    Expect.listEquals(src.asTypedList(n), dest.asTypedList(n));
+  }
+}

runtime/tests/vm/dart/typed_data_aot_regress43534_il_test.dart

@@ -26,9 +26,10 @@ void matchIL$main_foo(FlowGraph graph) {
   graph.match([
     match.block('Graph'),
     match.block('Function', [
-      match.LoadField(),
+      'list' << match.Parameter(index: 1),
+      match.LoadField('list', slot: 'TypedDataBase.length'),
       match.GenericCheckBound(),
-      match.LoadUntagged(),
+      match.LoadField('list', slot: 'PointerBase.data'),
       match.LoadIndexed(),
     ]),
   ]);

runtime/tests/vm/dart_2/typed_data_aot_regress43534_il_test.dart

@@ -26,9 +26,10 @@ void matchIL$main_foo(FlowGraph graph) {
   graph.match([
     match.block('Graph'),
     match.block('Function', [
-      match.LoadField(),
+      'list' << match.Parameter(index: 1),
+      match.LoadField('list', slot: 'TypedDataBase.length'),
       match.GenericCheckBound(),
-      match.LoadUntagged(),
+      match.LoadField('list', slot: 'PointerBase.data'),
       match.LoadIndexed(),
     ]),
   ]);

runtime/tools/ffi/sdk_lib_ffi_generator.dart

@@ -258,6 +258,7 @@ void generatePatchExtension(
       ? ""
       : """
   @patch
+  @pragma("vm:prefer-inline")
   $typedListType asTypedList(
     int length, {
      Pointer<NativeFinalizerFunction>? finalizer,

runtime/vm/bootstrap_natives.h

@@ -158,20 +158,6 @@ namespace dart {
   V(Timeline_getTraceClock, 0)                                                 \
   V(Timeline_isDartStreamEnabled, 0)                                           \
   V(Timeline_reportTaskEvent, 5)                                               \
-  V(TypedData_Int8Array_new, 2)                                                \
-  V(TypedData_Uint8Array_new, 2)                                               \
-  V(TypedData_Uint8ClampedArray_new, 2)                                        \
-  V(TypedData_Int16Array_new, 2)                                               \
-  V(TypedData_Uint16Array_new, 2)                                              \
-  V(TypedData_Int32Array_new, 2)                                               \
-  V(TypedData_Uint32Array_new, 2)                                              \
-  V(TypedData_Int64Array_new, 2)                                               \
-  V(TypedData_Uint64Array_new, 2)                                              \
-  V(TypedData_Float32Array_new, 2)                                             \
-  V(TypedData_Float64Array_new, 2)                                             \
-  V(TypedData_Float32x4Array_new, 2)                                           \
-  V(TypedData_Int32x4Array_new, 2)                                             \
-  V(TypedData_Float64x2Array_new, 2)                                           \
   V(TypedDataBase_length, 1)                                                   \
   V(TypedDataBase_setClampedRange, 5)                                          \
   V(TypedData_GetInt8, 2)                                                      \
@@ -200,38 +186,8 @@ namespace dart {
   V(TypedData_SetInt32x4, 3)                                                   \
   V(TypedData_GetFloat64x2, 2)                                                 \
   V(TypedData_SetFloat64x2, 3)                                                 \
-  V(TypedDataView_ByteDataView_new, 4)                                         \
-  V(TypedDataView_Int8ArrayView_new, 4)                                        \
-  V(TypedDataView_Uint8ArrayView_new, 4)                                       \
-  V(TypedDataView_Uint8ClampedArrayView_new, 4)                                \
-  V(TypedDataView_Int16ArrayView_new, 4)                                       \
-  V(TypedDataView_Uint16ArrayView_new, 4)                                      \
-  V(TypedDataView_Int32ArrayView_new, 4)                                       \
-  V(TypedDataView_Uint32ArrayView_new, 4)                                      \
-  V(TypedDataView_Int64ArrayView_new, 4)                                       \
-  V(TypedDataView_Uint64ArrayView_new, 4)                                      \
-  V(TypedDataView_Float32ArrayView_new, 4)                                     \
-  V(TypedDataView_Float64ArrayView_new, 4)                                     \
-  V(TypedDataView_Float32x4ArrayView_new, 4)                                   \
-  V(TypedDataView_Int32x4ArrayView_new, 4)                                     \
-  V(TypedDataView_Float64x2ArrayView_new, 4)                                   \
   V(TypedDataView_offsetInBytes, 1)                                            \
   V(TypedDataView_typedData, 1)                                                \
-  V(TypedDataView_UnmodifiableByteDataView_new, 4)                             \
-  V(TypedDataView_UnmodifiableInt8ArrayView_new, 4)                            \
-  V(TypedDataView_UnmodifiableUint8ArrayView_new, 4)                           \
-  V(TypedDataView_UnmodifiableUint8ClampedArrayView_new, 4)                    \
-  V(TypedDataView_UnmodifiableInt16ArrayView_new, 4)                           \
-  V(TypedDataView_UnmodifiableUint16ArrayView_new, 4)                          \
-  V(TypedDataView_UnmodifiableInt32ArrayView_new, 4)                           \
-  V(TypedDataView_UnmodifiableUint32ArrayView_new, 4)                          \
-  V(TypedDataView_UnmodifiableInt64ArrayView_new, 4)                           \
-  V(TypedDataView_UnmodifiableUint64ArrayView_new, 4)                          \
-  V(TypedDataView_UnmodifiableFloat32ArrayView_new, 4)                         \
-  V(TypedDataView_UnmodifiableFloat64ArrayView_new, 4)                         \
-  V(TypedDataView_UnmodifiableFloat32x4ArrayView_new, 4)                       \
-  V(TypedDataView_UnmodifiableInt32x4ArrayView_new, 4)                         \
-  V(TypedDataView_UnmodifiableFloat64x2ArrayView_new, 4)                       \
   V(Float32x4_fromDoubles, 4)                                                  \
   V(Float32x4_splat, 1)                                                        \
   V(Float32x4_fromInt32x4Bits, 2)                                              \