Skip to content

Commit

Permalink
Revert "[vm] Recognize unmodifiabled typed data views."
Browse files Browse the repository at this point in the history
This reverts commit d1112d3.

Reason for revert: b/242043014

Original change's description:
> [vm] Recognize unmodifiabled typed data views.
>
> These types now work with Dart_TypedDataAcquireData.
>
> The presence of these types no longer degrades the performance of typed data indexed loads.
>
> The presence of these types degrades the performance of typed data indexed stores much less. The performance of indexed stores is somewhat regressed if these types were not used.
>
> TEST=ci
> Bug: #32028
> Bug: #40924
> Bug: #42785
> Change-Id: I05ac5c9543f6f61ac37533b9efe511254778caed
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253700
> Reviewed-by: Aske Simon Christensen <askesc@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Commit-Queue: Ryan Macnak <rmacnak@google.com>

TBR=kustermann@google.com,rmacnak@google.com,askesc@google.com

TEST=ci
Change-Id: I32c1c460fc30c51bc0d42e7cfaafe72bf5630069
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: #32028
Bug: #40924
Bug: #42785
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254560
Reviewed-by: Siva Annamalai <asiva@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
  • Loading branch information
rmacnak-google authored and Commit Bot committed Aug 10, 2022
1 parent b7439ef commit 10bf1cf
Show file tree
Hide file tree
Showing 57 changed files with 3,006 additions and 5,886 deletions.
10 changes: 1 addition & 9 deletions runtime/lib/typed_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ static bool IsClamped(intptr_t cid) {
case kTypedDataUint8ClampedArrayCid:
case kExternalTypedDataUint8ClampedArrayCid:
case kTypedDataUint8ClampedArrayViewCid:
case kUnmodifiableTypedDataUint8ClampedArrayViewCid:
return true;
default:
return false;
Expand All @@ -106,11 +105,9 @@ static bool IsUint8(intptr_t cid) {
case kTypedDataUint8ClampedArrayCid:
case kExternalTypedDataUint8ClampedArrayCid:
case kTypedDataUint8ClampedArrayViewCid:
case kUnmodifiableTypedDataUint8ClampedArrayViewCid:
case kTypedDataUint8ArrayCid:
case kExternalTypedDataUint8ArrayCid:
case kTypedDataUint8ArrayViewCid:
case kUnmodifiableTypedDataUint8ArrayViewCid:
return true;
default:
return false;
Expand Down Expand Up @@ -189,15 +186,10 @@ static InstancePtr NewTypedDataView(intptr_t cid,
}

#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)
TYPED_DATA_VIEW_NEW(TypedDataView_##name##View_new, kTypedData##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

Expand Down
47 changes: 47 additions & 0 deletions runtime/tests/vm/dart/typed_data_aot_not_inlining_il_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) 2021, 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.

// This test asserts that we are not inlining accesses to typed data interfaces
// (e.g. Uint8List) if there are instantiated 3rd party classes (e.g.
// UnmodifiableUint8ListView).

import 'dart:typed_data';
import 'package:vm/testing/il_matchers.dart';

createThirdPartyUint8List() => UnmodifiableUint8ListView(Uint8List(10));

@pragma('vm:never-inline')
@pragma('vm:testing:print-flow-graph')
void foo(Uint8List list, int from) {
if (from >= list.length) {
list[from];
}
}

void matchIL$foo(FlowGraph graph) {
graph.match([
match.block('Graph'),
match.block('Function', [
'list' << match.Parameter(index: 0),
'from' << match.Parameter(index: 1),
'v13' << match.LoadClassId('list'),
match.PushArgument('list'),
match.DispatchTableCall('v13', selector_name: 'get:length'),
match.Branch(match.RelationalOp(match.any, match.any, kind: '>='),
ifTrue: 'B3'),
]),
'B3' <<
match.block('Target', [
'v15' << match.LoadClassId('list'),
match.PushArgument('list'),
match.PushArgument(/* BoxInt64(Parameter) or Parameter */),
match.DispatchTableCall('v15', selector_name: '[]'),
]),
]);
}

void main() {
foo(int.parse('1') == 1 ? createThirdPartyUint8List() : Uint8List(1),
int.parse('0'));
}
47 changes: 47 additions & 0 deletions runtime/tests/vm/dart_2/typed_data_aot_not_inlining_il_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) 2021, 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.

// This test asserts that we are not inlining accesses to typed data interfaces
// (e.g. Uint8List) if there are instantiated 3rd party classes (e.g.
// UnmodifiableUint8ListView).

import 'dart:typed_data';
import 'package:vm/testing/il_matchers.dart';

createThirdPartyUint8List() => UnmodifiableUint8ListView(Uint8List(10));

@pragma('vm:never-inline')
@pragma('vm:testing:print-flow-graph')
void foo(Uint8List list, int from) {
if (from >= list.length) {
list[from];
}
}

void matchIL$foo(FlowGraph graph) {
graph.match([
match.block('Graph'),
match.block('Function', [
'list' << match.Parameter(index: 0),
'from' << match.Parameter(index: 1),
'v13' << match.LoadClassId('list'),
match.PushArgument('list'),
match.DispatchTableCall('v13', selector_name: 'get:length'),
match.Branch(match.RelationalOp(match.any, match.any, kind: '>='),
ifTrue: 'B3'),
]),
'B3' <<
match.block('Target', [
'v15' << match.LoadClassId('list'),
match.PushArgument('list'),
match.PushArgument(/* BoxInt64(Parameter) or Parameter */),
match.DispatchTableCall('v15', selector_name: '[]'),
]),
]);
}

void main() {
foo(int.parse('1') == 1 ? createThirdPartyUint8List() : Uint8List(1),
int.parse('0'));
}
15 changes: 0 additions & 15 deletions runtime/vm/bootstrap_natives.h
Original file line number Diff line number Diff line change
Expand Up @@ -217,21 +217,6 @@ namespace dart {
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) \
Expand Down
58 changes: 22 additions & 36 deletions runtime/vm/class_id.h
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,10 @@ enum ClassId : intptr_t {
#define DEFINE_OBJECT_KIND(clazz) \
kTypedData##clazz##Cid, \
kTypedData##clazz##ViewCid, \
kExternalTypedData##clazz##Cid, \
kUnmodifiableTypedData##clazz##ViewCid,
kExternalTypedData##clazz##Cid,
CLASS_LIST_TYPED_DATA(DEFINE_OBJECT_KIND)
#undef DEFINE_OBJECT_KIND
kByteDataViewCid,
kUnmodifiableByteDataViewCid,

kByteBufferCid,
// clang-format on
Expand All @@ -257,7 +255,6 @@ enum ClassId : intptr_t {
const int kTypedDataCidRemainderInternal = 0;
const int kTypedDataCidRemainderView = 1;
const int kTypedDataCidRemainderExternal = 2;
const int kTypedDataCidRemainderUnmodifiable = 3;

// Class Id predicates.

Expand Down Expand Up @@ -364,45 +361,34 @@ inline bool IsTypeClassId(intptr_t index) {

inline bool IsTypedDataBaseClassId(intptr_t index) {
// Make sure this is updated when new TypedData types are added.
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 4 == kTypedDataUint8ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 3 == kTypedDataUint8ArrayCid);
return index >= kTypedDataInt8ArrayCid && index < kByteDataViewCid;
}

inline bool IsTypedDataClassId(intptr_t index) {
// Make sure this is updated when new TypedData types are added.
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 4 == kTypedDataUint8ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 3 == kTypedDataUint8ArrayCid);
return IsTypedDataBaseClassId(index) && ((index - kTypedDataInt8ArrayCid) %
4) == kTypedDataCidRemainderInternal;
3) == kTypedDataCidRemainderInternal;
}

inline bool IsTypedDataViewClassId(intptr_t index) {
// Make sure this is updated when new TypedData types are added.
COMPILE_ASSERT(kTypedDataInt8ArrayViewCid + 4 == kTypedDataUint8ArrayViewCid);
COMPILE_ASSERT(kTypedDataInt8ArrayViewCid + 3 == kTypedDataUint8ArrayViewCid);

const bool is_byte_data_view = index == kByteDataViewCid;
return is_byte_data_view ||
(IsTypedDataBaseClassId(index) &&
((index - kTypedDataInt8ArrayCid) % 4) == kTypedDataCidRemainderView);
((index - kTypedDataInt8ArrayCid) % 3) == kTypedDataCidRemainderView);
}

inline bool IsExternalTypedDataClassId(intptr_t index) {
// Make sure this is updated when new TypedData types are added.
COMPILE_ASSERT(kExternalTypedDataInt8ArrayCid + 4 ==
COMPILE_ASSERT(kExternalTypedDataInt8ArrayCid + 3 ==
kExternalTypedDataUint8ArrayCid);

return IsTypedDataBaseClassId(index) && ((index - kTypedDataInt8ArrayCid) %
4) == kTypedDataCidRemainderExternal;
}

inline bool IsUnmodifiableTypedDataViewClassId(intptr_t index) {
// Make sure this is updated when new TypedData types are added.
COMPILE_ASSERT(kExternalTypedDataInt8ArrayCid + 4 ==
kExternalTypedDataUint8ArrayCid);

const bool is_byte_data_view = index == kUnmodifiableByteDataViewCid;
return is_byte_data_view || (IsTypedDataBaseClassId(index) &&
((index - kTypedDataInt8ArrayCid) % 4) ==
kTypedDataCidRemainderUnmodifiable);
3) == kTypedDataCidRemainderExternal;
}

inline bool IsFfiTypeClassId(intptr_t index) {
Expand Down Expand Up @@ -460,21 +446,21 @@ COMPILE_ASSERT(kTypedDataInt8ArrayCid + 1 == kTypedDataInt8ArrayViewCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 2 == kExternalTypedDataInt8ArrayCid);

// Ensure the order of the typed data members in 3-step.
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 1 * 4 == kTypedDataUint8ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 2 * 4 ==
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 1 * 3 == kTypedDataUint8ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 2 * 3 ==
kTypedDataUint8ClampedArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 3 * 4 == kTypedDataInt16ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 4 * 4 == kTypedDataUint16ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 5 * 4 == kTypedDataInt32ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 6 * 4 == kTypedDataUint32ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 7 * 4 == kTypedDataInt64ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 8 * 4 == kTypedDataUint64ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 9 * 4 == kTypedDataFloat32ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 10 * 4 == kTypedDataFloat64ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 11 * 4 == kTypedDataFloat32x4ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 12 * 4 == kTypedDataInt32x4ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 13 * 4 == kTypedDataFloat64x2ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 14 * 4 == kByteDataViewCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 3 * 3 == kTypedDataInt16ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 4 * 3 == kTypedDataUint16ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 5 * 3 == kTypedDataInt32ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 6 * 3 == kTypedDataUint32ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 7 * 3 == kTypedDataInt64ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 8 * 3 == kTypedDataUint64ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 9 * 3 == kTypedDataFloat32ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 10 * 3 == kTypedDataFloat64ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 11 * 3 == kTypedDataFloat32x4ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 12 * 3 == kTypedDataInt32x4ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 13 * 3 == kTypedDataFloat64x2ArrayCid);
COMPILE_ASSERT(kTypedDataInt8ArrayCid + 14 * 3 == kByteDataViewCid);
COMPILE_ASSERT(kByteBufferCid + 1 == kNullCid);

} // namespace dart
Expand Down
7 changes: 0 additions & 7 deletions runtime/vm/compiler/backend/constant_propagator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -409,13 +409,6 @@ void ConstantPropagator::VisitGenericCheckBound(GenericCheckBoundInstr* instr) {
SetValue(instr, non_constant_);
}

void ConstantPropagator::VisitCheckWritable(CheckWritableInstr* instr) {
// Don't propagate constants through check, since it would eliminate
// the data dependence between the writable check and its use.
// Graph finalization will expose the constant eventually.
SetValue(instr, non_constant_);
}

void ConstantPropagator::VisitCheckNull(CheckNullInstr* instr) {
// Don't propagate constants through check, since it would eliminate
// the data dependence between the null check and its use.
Expand Down
30 changes: 0 additions & 30 deletions runtime/vm/compiler/backend/flow_graph_compiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3405,36 +3405,6 @@ void RangeErrorSlowPath::PushArgumentsForRuntimeCall(
locs->in(CheckBoundBase::kLengthPos).reg());
}

void RangeErrorSlowPath::EmitSharedStubCall(FlowGraphCompiler* compiler,
bool save_fpu_registers) {
#if defined(TARGET_ARCH_IA32)
UNREACHABLE();
#else
auto object_store = compiler->isolate_group()->object_store();
const auto& stub = Code::ZoneHandle(
compiler->zone(),
save_fpu_registers
? object_store->range_error_stub_with_fpu_regs_stub()
: object_store->range_error_stub_without_fpu_regs_stub());
compiler->EmitCallToStub(stub);
#endif
}

void WriteErrorSlowPath::EmitSharedStubCall(FlowGraphCompiler* compiler,
bool save_fpu_registers) {
#if defined(TARGET_ARCH_IA32)
UNREACHABLE();
#else
auto object_store = compiler->isolate_group()->object_store();
const auto& stub = Code::ZoneHandle(
compiler->zone(),
save_fpu_registers
? object_store->write_error_stub_with_fpu_regs_stub()
: object_store->write_error_stub_without_fpu_regs_stub());
compiler->EmitCallToStub(stub);
#endif
}

void LateInitializationErrorSlowPath::PushArgumentsForRuntimeCall(
FlowGraphCompiler* compiler) {
__ PushObject(Field::ZoneHandle(OriginalField()));
Expand Down
10 changes: 0 additions & 10 deletions runtime/vm/compiler/backend/flow_graph_compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -386,16 +386,6 @@ class RangeErrorSlowPath : public ThrowErrorSlowPathCode {
bool save_fpu_registers);
};

class WriteErrorSlowPath : public ThrowErrorSlowPathCode {
public:
explicit WriteErrorSlowPath(CheckWritableInstr* instruction)
: ThrowErrorSlowPathCode(instruction, kWriteErrorRuntimeEntry) {}
virtual const char* name() { return "check writable"; }

virtual void EmitSharedStubCall(FlowGraphCompiler* compiler,
bool save_fpu_registers);
};

class LateInitializationErrorSlowPath : public ThrowErrorSlowPathCode {
public:
explicit LateInitializationErrorSlowPath(Instruction* instruction)
Expand Down
Loading

0 comments on commit 10bf1cf

Please sign in to comment.