Skip to content

Commit 5362d95

Browse files
alexmarkovCommit Queue
authored and
Commit Queue
committed
Revert "[vm/compiler] Change MemoryCopy to also take untagged addresses."
This reverts commit 06d7a23. Reason for revert: everything crashes on vm-aot-linux-debug-simarm_x64 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]> Issue: #42072 Change-Id: I7c31434e01108487de69a32154bbefd1538c6f0f 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 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/330523 Bot-Commit: Rubber Stamper <[email protected]> Commit-Queue: Alexander Aprelev <[email protected]> Auto-Submit: Alexander Markov <[email protected]> Reviewed-by: Alexander Aprelev <[email protected]>
1 parent 06d7a23 commit 5362d95

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+718
-1036
lines changed

pkg/vm/lib/testing/il_matchers.dart

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -183,10 +183,6 @@ class Env {
183183
void bind(String name, Map<String, dynamic> instrOrBlock) {
184184
final id = instrOrBlock['v'] ?? instrOrBlock['b'];
185185

186-
if (id == null) {
187-
throw 'Instruction is not a definition or a block: ${instrOrBlock['o']}';
188-
}
189-
190186
if (nameToId.containsKey(name)) {
191187
if (nameToId[name] != id) {
192188
throw 'Binding mismatch for $name: got ${nameToId[name]} and $id';

runtime/lib/typed_data.cc

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,24 @@ static void RangeCheck(intptr_t offset_in_bytes,
2929
}
3030
}
3131

32+
static void AlignmentCheck(intptr_t offset_in_bytes, intptr_t element_size) {
33+
if ((offset_in_bytes % element_size) != 0) {
34+
const auto& error = String::Handle(String::NewFormatted(
35+
"Offset in bytes (%" Pd ") must be a multiple of %" Pd "",
36+
offset_in_bytes, element_size));
37+
Exceptions::ThrowArgumentError(error);
38+
}
39+
}
40+
41+
// Checks to see if a length will not result in an OOM error.
42+
static void LengthCheck(intptr_t len, intptr_t max) {
43+
if (len < 0 || len > max) {
44+
const String& error = String::Handle(String::NewFormatted(
45+
"Length (%" Pd ") of object must be in range [0..%" Pd "]", len, max));
46+
Exceptions::ThrowArgumentError(error);
47+
}
48+
}
49+
3250
DEFINE_NATIVE_ENTRY(TypedDataBase_length, 0, 1) {
3351
GET_NON_NULL_NATIVE_ARGUMENT(TypedDataBase, array, arguments->NativeArgAt(0));
3452
return Smi::New(array.Length());
@@ -130,6 +148,67 @@ DEFINE_NATIVE_ENTRY(TypedDataBase_setClampedRange, 0, 5) {
130148
return Object::null();
131149
}
132150

151+
// Native methods for typed data allocation are recognized and implemented
152+
// in FlowGraphBuilder::BuildGraphOfRecognizedMethod.
153+
// These bodies exist only to assert that they are not used.
154+
#define TYPED_DATA_NEW(name) \
155+
DEFINE_NATIVE_ENTRY(TypedData_##name##_new, 0, 2) { \
156+
UNREACHABLE(); \
157+
return Object::null(); \
158+
}
159+
160+
#define TYPED_DATA_NEW_NATIVE(name) TYPED_DATA_NEW(name)
161+
162+
CLASS_LIST_TYPED_DATA(TYPED_DATA_NEW_NATIVE)
163+
#undef TYPED_DATA_NEW_NATIVE
164+
#undef TYPED_DATA_NEW
165+
166+
// We check the length parameter against a possible maximum length for the
167+
// array based on available physical addressable memory on the system.
168+
//
169+
// More specifically
170+
//
171+
// TypedData::MaxElements(cid) is equal to (kSmiMax / ElementSizeInBytes(cid))
172+
//
173+
// which ensures that the number of bytes the array holds is guaranteed to fit
174+
// into a _Smi.
175+
//
176+
// Argument 0 is type arguments and is ignored.
177+
static InstancePtr NewTypedDataView(intptr_t cid,
178+
intptr_t element_size,
179+
Zone* zone,
180+
NativeArguments* arguments) {
181+
GET_NON_NULL_NATIVE_ARGUMENT(TypedDataBase, typed_data,
182+
arguments->NativeArgAt(1));
183+
GET_NON_NULL_NATIVE_ARGUMENT(Smi, offset, arguments->NativeArgAt(2));
184+
GET_NON_NULL_NATIVE_ARGUMENT(Smi, len, arguments->NativeArgAt(3));
185+
const intptr_t backing_length = typed_data.LengthInBytes();
186+
const intptr_t offset_in_bytes = offset.Value();
187+
const intptr_t length = len.Value();
188+
AlignmentCheck(offset_in_bytes, element_size);
189+
LengthCheck(offset_in_bytes + length * element_size, backing_length);
190+
return TypedDataView::New(cid, typed_data, offset_in_bytes, length);
191+
}
192+
193+
#define TYPED_DATA_VIEW_NEW(native_name, cid) \
194+
DEFINE_NATIVE_ENTRY(native_name, 0, 4) { \
195+
return NewTypedDataView(cid, TypedDataBase::ElementSizeInBytes(cid), zone, \
196+
arguments); \
197+
}
198+
199+
#define TYPED_DATA_NEW_NATIVE(name) \
200+
TYPED_DATA_VIEW_NEW(TypedDataView_##name##View_new, \
201+
kTypedData##name##ViewCid) \
202+
TYPED_DATA_VIEW_NEW(TypedDataView_Unmodifiable##name##View_new, \
203+
kUnmodifiableTypedData##name##ViewCid)
204+
205+
CLASS_LIST_TYPED_DATA(TYPED_DATA_NEW_NATIVE)
206+
TYPED_DATA_VIEW_NEW(TypedDataView_ByteDataView_new, kByteDataViewCid)
207+
TYPED_DATA_VIEW_NEW(TypedDataView_UnmodifiableByteDataView_new,
208+
kUnmodifiableByteDataViewCid)
209+
#undef TYPED_DATA_NEW_NATIVE
210+
#undef TYPED_DATA_VIEW_NEW
211+
133212
#define TYPED_DATA_GETTER(getter, object, ctor, access_size) \
134213
DEFINE_NATIVE_ENTRY(TypedData_##getter, 0, 2) { \
135214
GET_NON_NULL_NATIVE_ARGUMENT(TypedDataBase, array, \

runtime/tests/vm/dart/address_local_pointer_il_test.dart

Lines changed: 0 additions & 33 deletions
This file was deleted.

runtime/tests/vm/dart/pointer_as_typed_list_il_test.dart

Lines changed: 0 additions & 58 deletions
This file was deleted.

runtime/tests/vm/dart/typed_data_aot_regress43534_il_test.dart

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,9 @@ void matchIL$main_foo(FlowGraph graph) {
2626
graph.match([
2727
match.block('Graph'),
2828
match.block('Function', [
29-
'list' << match.Parameter(index: 1),
30-
match.LoadField('list', slot: 'TypedDataBase.length'),
29+
match.LoadField(),
3130
match.GenericCheckBound(),
32-
match.LoadField('list', slot: 'PointerBase.data'),
31+
match.LoadUntagged(),
3332
match.LoadIndexed(),
3433
]),
3534
]);

runtime/tests/vm/dart_2/typed_data_aot_regress43534_il_test.dart

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,9 @@ void matchIL$main_foo(FlowGraph graph) {
2626
graph.match([
2727
match.block('Graph'),
2828
match.block('Function', [
29-
'list' << match.Parameter(index: 1),
30-
match.LoadField('list', slot: 'TypedDataBase.length'),
29+
match.LoadField(),
3130
match.GenericCheckBound(),
32-
match.LoadField('list', slot: 'PointerBase.data'),
31+
match.LoadUntagged(),
3332
match.LoadIndexed(),
3433
]),
3534
]);

runtime/tools/ffi/sdk_lib_ffi_generator.dart

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,6 @@ void generatePatchExtension(
258258
? ""
259259
: """
260260
@patch
261-
@pragma("vm:prefer-inline")
262261
$typedListType asTypedList(
263262
int length, {
264263
Pointer<NativeFinalizerFunction>? finalizer,

runtime/vm/bootstrap_natives.h

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,20 @@ namespace dart {
158158
V(Timeline_getTraceClock, 0) \
159159
V(Timeline_isDartStreamEnabled, 0) \
160160
V(Timeline_reportTaskEvent, 5) \
161+
V(TypedData_Int8Array_new, 2) \
162+
V(TypedData_Uint8Array_new, 2) \
163+
V(TypedData_Uint8ClampedArray_new, 2) \
164+
V(TypedData_Int16Array_new, 2) \
165+
V(TypedData_Uint16Array_new, 2) \
166+
V(TypedData_Int32Array_new, 2) \
167+
V(TypedData_Uint32Array_new, 2) \
168+
V(TypedData_Int64Array_new, 2) \
169+
V(TypedData_Uint64Array_new, 2) \
170+
V(TypedData_Float32Array_new, 2) \
171+
V(TypedData_Float64Array_new, 2) \
172+
V(TypedData_Float32x4Array_new, 2) \
173+
V(TypedData_Int32x4Array_new, 2) \
174+
V(TypedData_Float64x2Array_new, 2) \
161175
V(TypedDataBase_length, 1) \
162176
V(TypedDataBase_setClampedRange, 5) \
163177
V(TypedData_GetInt8, 2) \
@@ -186,8 +200,38 @@ namespace dart {
186200
V(TypedData_SetInt32x4, 3) \
187201
V(TypedData_GetFloat64x2, 2) \
188202
V(TypedData_SetFloat64x2, 3) \
203+
V(TypedDataView_ByteDataView_new, 4) \
204+
V(TypedDataView_Int8ArrayView_new, 4) \
205+
V(TypedDataView_Uint8ArrayView_new, 4) \
206+
V(TypedDataView_Uint8ClampedArrayView_new, 4) \
207+
V(TypedDataView_Int16ArrayView_new, 4) \
208+
V(TypedDataView_Uint16ArrayView_new, 4) \
209+
V(TypedDataView_Int32ArrayView_new, 4) \
210+
V(TypedDataView_Uint32ArrayView_new, 4) \
211+
V(TypedDataView_Int64ArrayView_new, 4) \
212+
V(TypedDataView_Uint64ArrayView_new, 4) \
213+
V(TypedDataView_Float32ArrayView_new, 4) \
214+
V(TypedDataView_Float64ArrayView_new, 4) \
215+
V(TypedDataView_Float32x4ArrayView_new, 4) \
216+
V(TypedDataView_Int32x4ArrayView_new, 4) \
217+
V(TypedDataView_Float64x2ArrayView_new, 4) \
189218
V(TypedDataView_offsetInBytes, 1) \
190219
V(TypedDataView_typedData, 1) \
220+
V(TypedDataView_UnmodifiableByteDataView_new, 4) \
221+
V(TypedDataView_UnmodifiableInt8ArrayView_new, 4) \
222+
V(TypedDataView_UnmodifiableUint8ArrayView_new, 4) \
223+
V(TypedDataView_UnmodifiableUint8ClampedArrayView_new, 4) \
224+
V(TypedDataView_UnmodifiableInt16ArrayView_new, 4) \
225+
V(TypedDataView_UnmodifiableUint16ArrayView_new, 4) \
226+
V(TypedDataView_UnmodifiableInt32ArrayView_new, 4) \
227+
V(TypedDataView_UnmodifiableUint32ArrayView_new, 4) \
228+
V(TypedDataView_UnmodifiableInt64ArrayView_new, 4) \
229+
V(TypedDataView_UnmodifiableUint64ArrayView_new, 4) \
230+
V(TypedDataView_UnmodifiableFloat32ArrayView_new, 4) \
231+
V(TypedDataView_UnmodifiableFloat64ArrayView_new, 4) \
232+
V(TypedDataView_UnmodifiableFloat32x4ArrayView_new, 4) \
233+
V(TypedDataView_UnmodifiableInt32x4ArrayView_new, 4) \
234+
V(TypedDataView_UnmodifiableFloat64x2ArrayView_new, 4) \
191235
V(Float32x4_fromDoubles, 4) \
192236
V(Float32x4_splat, 1) \
193237
V(Float32x4_fromInt32x4Bits, 2) \

runtime/vm/compiler/backend/compile_type.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,6 @@ class CompileType : public ZoneAllocated {
191191
// Create non-nullable String type.
192192
static CompileType String();
193193

194-
// Create non-nullable Object type.
195-
static CompileType Object();
196-
197194
// Perform a join operation over the type lattice.
198195
void Union(CompileType* other);
199196

0 commit comments

Comments
 (0)