Skip to content

Commit ad95bb6

Browse files
alexmarkovCommit Queue
alexmarkov
authored and
Commit Queue
committed
Reland "[vm] Implement typed data view GetIndexed in flow graph builder"
This is a reland of commit a7e2dce Fixes the crash during code generation when typed data view LoadIndexed was used on an internal typed data array (in the unreachable code path) without loading its payload. TEST=runtime/tests/vm/dart/regress_b_334128316_test.dart TEST=manually verified repro from b/334128316 Original change's description: > [vm] Implement typed data view GetIndexed in flow graph builder > > Typed data view 'operator []' is now implemented in the flow > graph builder. This unifies all typed data GetIndexed operations > and removes unnecessary address calculations. > > TEST=ci > > Change-Id: I8d2ca6e7c7bf18b2590536a643f92cad5beb6d95 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/361283 > Reviewed-by: Tess Strickland <[email protected]> > Commit-Queue: Alexander Markov <[email protected]> > Reviewed-by: Slava Egorov <[email protected]> Change-Id: I3786fa17e971f8ba6cd01a57f1a68504bf24bc47 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362821 Reviewed-by: Tess Strickland <[email protected]> Commit-Queue: Alexander Markov <[email protected]> Reviewed-by: Slava Egorov <[email protected]>
1 parent 95d8710 commit ad95bb6

File tree

9 files changed

+212
-315
lines changed

9 files changed

+212
-315
lines changed

‎runtime/tests/vm/dart/regress_b_334128316_test.dart

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// Regression test for b/334128316.
6+
//
7+
// Verifies that compiler doesn't crash when generating code
8+
// for typed data view LoadIndexed which takes an internal typed data
9+
// (in unreachable code path).
10+
11+
// VMOptions=--deterministic
12+
13+
import 'dart:typed_data';
14+
15+
@pragma('vm:never-inline')
16+
int foo(Uint8List bytes, int n) {
17+
int sum = 0;
18+
for (int i = 0; i < n - 2; ++i) {
19+
// Polymorphic call, 2 targets.
20+
// One of the targets is incompatible with CheckClass,
21+
// creating an impossible LoadIndexed in the unreachable code.
22+
int b0 = bytes[i];
23+
if (b0 == 1) {
24+
// Monomorphic call to [].
25+
// CheckClass from this inline is moved out of the loop.
26+
int b1 = bytes[i + 1];
27+
sum += b1;
28+
}
29+
}
30+
return sum;
31+
}
32+
33+
void main() {
34+
Uint8List input1 = Uint8List.view(Uint8List(10).buffer, 2);
35+
Uint8List input2 = Uint8List(10);
36+
for (int i = 0; i < input2.length; ++i) {
37+
input2[i] = 1;
38+
}
39+
for (int i = 0; i < 1000; ++i) {
40+
// Ensure certain order during polymorphic inlining by
41+
// using 2x more views than internal typed data.
42+
foo(input1, input1.length);
43+
foo(input1, input1.length);
44+
foo(input2, input2.length);
45+
}
46+
}

‎runtime/tests/vm/dart/typed_list_index_checkbound_il_test.dart

Lines changed: 15 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,13 @@ int retrieveFromView(Int8List src, int n) => src[n];
1717

1818
@pragma('vm:never-inline')
1919
@pragma('vm:testing:print-flow-graph')
20-
int retrieveFromBase(Int8List src, int n) => src[n];
20+
int retrieveFromInternal(Int8List src, int n) => src[n];
2121

2222
@pragma('vm:never-inline')
2323
@pragma('vm:testing:print-flow-graph')
2424
int retrieveFromExternal(Int8List src, int n) => src[n];
2525

26-
void matchIL$retrieveFromView(FlowGraph graph) {
26+
void matchILRetrieveFromNonInternal(FlowGraph graph) {
2727
graph.match([
2828
match.block('Graph'),
2929
match.block('Function', [
@@ -37,49 +37,27 @@ void matchIL$retrieveFromView(FlowGraph graph) {
3737
'unboxed_len' << match.UnboxInt64('len'),
3838
match.GenericCheckBound('unboxed_len', 'n'),
3939
],
40-
'typed_data' << match.LoadField('src', slot: 'TypedDataView.typed_data'),
41-
'boxed_offset' <<
42-
match.LoadField('src', slot: 'TypedDataView.offset_in_bytes'),
43-
'offset' << match.UnboxInt64('boxed_offset'),
44-
'index' << match.BinaryInt64Op('offset', 'n', op_kind: '+'),
45-
if (is32BitConfiguration) ...[
46-
'boxed_index' << match.BoxInt64('index'),
47-
],
48-
'data' << match.LoadField('typed_data', slot: 'PointerBase.data'),
40+
'data' << match.LoadField('src', slot: 'PointerBase.data'),
4941
if (is32BitConfiguration) ...[
50-
'retval32' << match.LoadIndexed('data', 'boxed_index'),
42+
'retval32' << match.LoadIndexed('data', 'boxed_n'),
5143
'retval' << match.IntConverter('retval32', from: 'int32', to: 'int64'),
5244
] else ...[
53-
'retval' << match.LoadIndexed('data', 'index'),
45+
'retval' << match.LoadIndexed('data', 'n'),
5446
],
5547
match.DartReturn('retval'),
5648
]),
5749
]);
5850
}
5951

60-
void matchIL$retrieveFromBase(FlowGraph graph) {
61-
graph.match([
62-
match.block('Graph'),
63-
match.block('Function', [
64-
'src' << match.Parameter(index: 0),
65-
'n' << match.Parameter(index: 1),
66-
'len' << match.LoadField('src', slot: 'TypedDataBase.length'),
67-
if (is32BitConfiguration) ...[
68-
'boxed_n' << match.BoxInt64('n'),
69-
match.GenericCheckBound('len', 'boxed_n'),
70-
'retval32' << match.LoadIndexed('src', 'boxed_n'),
71-
'retval' << match.IntConverter('retval32', from: 'int32', to: 'int64'),
72-
] else ...[
73-
'unboxed_len' << match.UnboxInt64('len'),
74-
match.GenericCheckBound('unboxed_len', 'n'),
75-
'retval' << match.LoadIndexed('src', 'n'),
76-
],
77-
match.DartReturn('retval'),
78-
]),
79-
]);
52+
void matchIL$retrieveFromView(FlowGraph graph) {
53+
matchILRetrieveFromNonInternal(graph);
8054
}
8155

8256
void matchIL$retrieveFromExternal(FlowGraph graph) {
57+
matchILRetrieveFromNonInternal(graph);
58+
}
59+
60+
void matchIL$retrieveFromInternal(FlowGraph graph) {
8361
graph.match([
8462
match.block('Graph'),
8563
match.block('Function', [
@@ -89,16 +67,12 @@ void matchIL$retrieveFromExternal(FlowGraph graph) {
8967
if (is32BitConfiguration) ...[
9068
'boxed_n' << match.BoxInt64('n'),
9169
match.GenericCheckBound('len', 'boxed_n'),
70+
'retval32' << match.LoadIndexed('src', 'boxed_n'),
71+
'retval' << match.IntConverter('retval32', from: 'int32', to: 'int64'),
9272
] else ...[
9373
'unboxed_len' << match.UnboxInt64('len'),
9474
match.GenericCheckBound('unboxed_len', 'n'),
95-
],
96-
'data' << match.LoadField('src', slot: 'PointerBase.data'),
97-
if (is32BitConfiguration) ...[
98-
'retval32' << match.LoadIndexed('data', 'boxed_n'),
99-
'retval' << match.IntConverter('retval32', from: 'int32', to: 'int64'),
100-
] else ...[
101-
'retval' << match.LoadIndexed('data', 'n'),
75+
'retval' << match.LoadIndexed('src', 'n'),
10276
],
10377
match.DartReturn('retval'),
10478
]),
@@ -108,7 +82,7 @@ void matchIL$retrieveFromExternal(FlowGraph graph) {
10882
void main(List<String> args) {
10983
final n = args.isEmpty ? 0 : int.parse(args.first);
11084
final list = Int8List.fromList([1, 2, 3, 4]);
111-
print(retrieveFromBase(list, n));
85+
print(retrieveFromInternal(list, n));
11286
print(retrieveFromView(Int8List.sublistView(list), n));
11387
if (!isSimulator) {
11488
using((arena) {

‎runtime/vm/compiler/backend/flow_graph.cc

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2648,11 +2648,18 @@ void FlowGraph::ExtractNonInternalTypedDataPayload(Instruction* instr,
26482648
auto const type_cid = array->Type()->ToCid();
26492649
// For external PointerBase objects, the payload should have already been
26502650
// extracted during canonicalization.
2651-
ASSERT(!IsExternalPayloadClassId(cid) || !IsExternalPayloadClassId(type_cid));
2652-
// Don't extract if the array is an internal typed data object.
2653-
if (IsTypedDataClassId(type_cid)) return;
2654-
ExtractUntaggedPayload(instr, array, Slot::PointerBase_data(),
2655-
InnerPointerAccess::kMayBeInnerPointer);
2651+
ASSERT(!IsExternalPayloadClassId(cid) && !IsExternalPayloadClassId(type_cid));
2652+
// Extract payload for typed data view instructions even if array is
2653+
// an internal typed data (could happen in the unreachable code),
2654+
// as code generation handles direct accesses only for internal typed data.
2655+
//
2656+
// For internal typed data instructions (which are also used for
2657+
// non-internal typed data arrays), don't extract payload if the array is
2658+
// an internal typed data object.
2659+
if (IsTypedDataViewClassId(cid) || !IsTypedDataClassId(type_cid)) {
2660+
ExtractUntaggedPayload(instr, array, Slot::PointerBase_data(),
2661+
InnerPointerAccess::kMayBeInnerPointer);
2662+
}
26562663
}
26572664

26582665
void FlowGraph::ExtractNonInternalTypedDataPayloads() {

‎runtime/vm/compiler/backend/redundancy_elimination.cc

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1110,31 +1110,41 @@ class AliasedSet : public ZoneAllocated {
11101110
} else if (UseIsARedefinition(use) &&
11111111
AnyUseCreatesAlias(instr->Cast<Definition>())) {
11121112
return true;
1113-
} else if ((instr->IsStoreField() &&
1114-
(use->use_index() != StoreFieldInstr::kInstancePos))) {
1115-
ASSERT(use->use_index() == StoreFieldInstr::kValuePos);
1116-
// If we store this value into an object that is not aliased itself
1117-
// and we never load again then the store does not create an alias.
1113+
} else if (instr->IsStoreField()) {
11181114
StoreFieldInstr* store = instr->AsStoreField();
1119-
Definition* instance =
1120-
store->instance()->definition()->OriginalDefinition();
1121-
if (Place::IsAllocation(instance) &&
1122-
!instance->Identity().IsAliased()) {
1123-
bool is_load, is_store;
1124-
Place store_place(instr, &is_load, &is_store);
1125-
1126-
if (!HasLoadsFromPlace(instance, &store_place)) {
1127-
// No loads found that match this store. If it is yet unknown if
1128-
// the object is not aliased then optimistically assume this but
1129-
// add it to the worklist to check its uses transitively.
1130-
if (instance->Identity().IsUnknown()) {
1131-
instance->SetIdentity(AliasIdentity::NotAliased());
1132-
aliasing_worklist_.Add(instance);
1115+
1116+
if (store->slot().kind() == Slot::Kind::kTypedDataView_typed_data) {
1117+
// Initialization of TypedDataView.typed_data field creates
1118+
// aliasing between the view and original typed data,
1119+
// as the same data can now be accessed via both typed data
1120+
// view and the original typed data.
1121+
return true;
1122+
}
1123+
1124+
if (use->use_index() != StoreFieldInstr::kInstancePos) {
1125+
ASSERT(use->use_index() == StoreFieldInstr::kValuePos);
1126+
// If we store this value into an object that is not aliased itself
1127+
// and we never load again then the store does not create an alias.
1128+
Definition* instance =
1129+
store->instance()->definition()->OriginalDefinition();
1130+
if (Place::IsAllocation(instance) &&
1131+
!instance->Identity().IsAliased()) {
1132+
bool is_load, is_store;
1133+
Place store_place(instr, &is_load, &is_store);
1134+
1135+
if (!HasLoadsFromPlace(instance, &store_place)) {
1136+
// No loads found that match this store. If it is yet unknown if
1137+
// the object is not aliased then optimistically assume this but
1138+
// add it to the worklist to check its uses transitively.
1139+
if (instance->Identity().IsUnknown()) {
1140+
instance->SetIdentity(AliasIdentity::NotAliased());
1141+
aliasing_worklist_.Add(instance);
1142+
}
1143+
continue;
11331144
}
1134-
continue;
11351145
}
1146+
return true;
11361147
}
1137-
return true;
11381148
} else if (auto* const alloc = instr->AsAllocation()) {
11391149
// Treat inputs to an allocation instruction exactly as if they were
11401150
// manually stored using a StoreField instruction.

‎runtime/vm/compiler/frontend/kernel_to_il.cc

Lines changed: 19 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -960,36 +960,17 @@ bool FlowGraphBuilder::IsRecognizedMethodForFlowGraph(
960960
const MethodRecognizer::Kind kind = function.recognized_kind();
961961

962962
switch (kind) {
963+
#define TYPED_DATA_GET_INDEXED_CASES(clazz) \
964+
case MethodRecognizer::k##clazz##ArrayGetIndexed: \
965+
FALL_THROUGH; \
966+
case MethodRecognizer::kExternal##clazz##ArrayGetIndexed: \
967+
FALL_THROUGH; \
968+
case MethodRecognizer::k##clazz##ArrayViewGetIndexed: \
969+
FALL_THROUGH;
970+
DART_CLASS_LIST_TYPED_DATA(TYPED_DATA_GET_INDEXED_CASES)
971+
#undef TYPED_DATA_GET_INDEXED_CASES
963972
case MethodRecognizer::kObjectArrayGetIndexed:
964973
case MethodRecognizer::kGrowableArrayGetIndexed:
965-
case MethodRecognizer::kInt8ArrayGetIndexed:
966-
case MethodRecognizer::kExternalInt8ArrayGetIndexed:
967-
case MethodRecognizer::kUint8ArrayGetIndexed:
968-
case MethodRecognizer::kExternalUint8ArrayGetIndexed:
969-
case MethodRecognizer::kUint8ClampedArrayGetIndexed:
970-
case MethodRecognizer::kExternalUint8ClampedArrayGetIndexed:
971-
case MethodRecognizer::kInt16ArrayGetIndexed:
972-
case MethodRecognizer::kExternalInt16ArrayGetIndexed:
973-
case MethodRecognizer::kUint16ArrayGetIndexed:
974-
case MethodRecognizer::kExternalUint16ArrayGetIndexed:
975-
case MethodRecognizer::kInt32ArrayGetIndexed:
976-
case MethodRecognizer::kExternalInt32ArrayGetIndexed:
977-
case MethodRecognizer::kUint32ArrayGetIndexed:
978-
case MethodRecognizer::kExternalUint32ArrayGetIndexed:
979-
case MethodRecognizer::kInt64ArrayGetIndexed:
980-
case MethodRecognizer::kExternalInt64ArrayGetIndexed:
981-
case MethodRecognizer::kUint64ArrayGetIndexed:
982-
case MethodRecognizer::kExternalUint64ArrayGetIndexed:
983-
case MethodRecognizer::kFloat32ArrayGetIndexed:
984-
case MethodRecognizer::kExternalFloat32ArrayGetIndexed:
985-
case MethodRecognizer::kFloat64ArrayGetIndexed:
986-
case MethodRecognizer::kExternalFloat64ArrayGetIndexed:
987-
case MethodRecognizer::kFloat32x4ArrayGetIndexed:
988-
case MethodRecognizer::kExternalFloat32x4ArrayGetIndexed:
989-
case MethodRecognizer::kFloat64x2ArrayGetIndexed:
990-
case MethodRecognizer::kExternalFloat64x2ArrayGetIndexed:
991-
case MethodRecognizer::kInt32x4ArrayGetIndexed:
992-
case MethodRecognizer::kExternalInt32x4ArrayGetIndexed:
993974
case MethodRecognizer::kRecord_fieldAt:
994975
case MethodRecognizer::kRecord_fieldNames:
995976
case MethodRecognizer::kRecord_numFields:
@@ -1214,36 +1195,17 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
12141195

12151196
const MethodRecognizer::Kind kind = function.recognized_kind();
12161197
switch (kind) {
1198+
#define TYPED_DATA_GET_INDEXED_CASES(clazz) \
1199+
case MethodRecognizer::k##clazz##ArrayGetIndexed: \
1200+
FALL_THROUGH; \
1201+
case MethodRecognizer::kExternal##clazz##ArrayGetIndexed: \
1202+
FALL_THROUGH; \
1203+
case MethodRecognizer::k##clazz##ArrayViewGetIndexed: \
1204+
FALL_THROUGH;
1205+
DART_CLASS_LIST_TYPED_DATA(TYPED_DATA_GET_INDEXED_CASES)
1206+
#undef TYPED_DATA_GET_INDEXED_CASES
12171207
case MethodRecognizer::kObjectArrayGetIndexed:
1218-
case MethodRecognizer::kGrowableArrayGetIndexed:
1219-
case MethodRecognizer::kInt8ArrayGetIndexed:
1220-
case MethodRecognizer::kExternalInt8ArrayGetIndexed:
1221-
case MethodRecognizer::kUint8ArrayGetIndexed:
1222-
case MethodRecognizer::kExternalUint8ArrayGetIndexed:
1223-
case MethodRecognizer::kUint8ClampedArrayGetIndexed:
1224-
case MethodRecognizer::kExternalUint8ClampedArrayGetIndexed:
1225-
case MethodRecognizer::kInt16ArrayGetIndexed:
1226-
case MethodRecognizer::kExternalInt16ArrayGetIndexed:
1227-
case MethodRecognizer::kUint16ArrayGetIndexed:
1228-
case MethodRecognizer::kExternalUint16ArrayGetIndexed:
1229-
case MethodRecognizer::kInt32ArrayGetIndexed:
1230-
case MethodRecognizer::kExternalInt32ArrayGetIndexed:
1231-
case MethodRecognizer::kUint32ArrayGetIndexed:
1232-
case MethodRecognizer::kExternalUint32ArrayGetIndexed:
1233-
case MethodRecognizer::kInt64ArrayGetIndexed:
1234-
case MethodRecognizer::kExternalInt64ArrayGetIndexed:
1235-
case MethodRecognizer::kUint64ArrayGetIndexed:
1236-
case MethodRecognizer::kExternalUint64ArrayGetIndexed:
1237-
case MethodRecognizer::kFloat32ArrayGetIndexed:
1238-
case MethodRecognizer::kExternalFloat32ArrayGetIndexed:
1239-
case MethodRecognizer::kFloat64ArrayGetIndexed:
1240-
case MethodRecognizer::kExternalFloat64ArrayGetIndexed:
1241-
case MethodRecognizer::kFloat32x4ArrayGetIndexed:
1242-
case MethodRecognizer::kExternalFloat32x4ArrayGetIndexed:
1243-
case MethodRecognizer::kFloat64x2ArrayGetIndexed:
1244-
case MethodRecognizer::kExternalFloat64x2ArrayGetIndexed:
1245-
case MethodRecognizer::kInt32x4ArrayGetIndexed:
1246-
case MethodRecognizer::kExternalInt32x4ArrayGetIndexed: {
1208+
case MethodRecognizer::kGrowableArrayGetIndexed: {
12471209
ASSERT_EQUAL(function.NumParameters(), 2);
12481210
intptr_t array_cid = MethodRecognizer::MethodKindToReceiverCid(kind);
12491211
const Representation elem_rep =

0 commit comments

Comments
 (0)