[vm/compiler] Forward typed data view constructor arguments.
Also adds tests for load optimizer fixes in 836c04f.
Bug: https://github.com/dart-lang/sdk/issues/36570
Change-Id: Ie8ea7f944bc789b4c0894f9d96010113fb3ec4b6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99466
Commit-Queue: Teagan Strickland <sstrickl@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
index df912de..dee6cab 100644
--- a/runtime/vm/compiler/backend/il.cc
+++ b/runtime/vm/compiler/backend/il.cc
@@ -2632,6 +2632,30 @@
}
}
+bool LoadFieldInstr::IsTypedDataViewFactory(const Function& function) {
+ auto kind = MethodRecognizer::RecognizeKind(function);
+ switch (kind) {
+ case MethodRecognizer::kTypedData_ByteDataView_factory:
+ case MethodRecognizer::kTypedData_Int8ArrayView_factory:
+ case MethodRecognizer::kTypedData_Uint8ArrayView_factory:
+ case MethodRecognizer::kTypedData_Uint8ClampedArrayView_factory:
+ case MethodRecognizer::kTypedData_Int16ArrayView_factory:
+ case MethodRecognizer::kTypedData_Uint16ArrayView_factory:
+ case MethodRecognizer::kTypedData_Int32ArrayView_factory:
+ case MethodRecognizer::kTypedData_Uint32ArrayView_factory:
+ case MethodRecognizer::kTypedData_Int64ArrayView_factory:
+ case MethodRecognizer::kTypedData_Uint64ArrayView_factory:
+ case MethodRecognizer::kTypedData_Float32ArrayView_factory:
+ case MethodRecognizer::kTypedData_Float64ArrayView_factory:
+ case MethodRecognizer::kTypedData_Float32x4ArrayView_factory:
+ case MethodRecognizer::kTypedData_Int32x4ArrayView_factory:
+ case MethodRecognizer::kTypedData_Float64x2ArrayView_factory:
+ return true;
+ default:
+ return false;
+ }
+}
+
Definition* ConstantInstr::Canonicalize(FlowGraph* flow_graph) {
return HasUses() ? this : NULL;
}
@@ -2703,6 +2727,16 @@
if (call->is_known_list_constructor() &&
IsFixedLengthArrayCid(call->Type()->ToCid())) {
return call->ArgumentAt(1);
+ } else if (IsTypedDataViewFactory(call->function())) {
+ // Typed data view factories all take three arguments (after
+ // the implicit type arguments parameter):
+ //
+ // 1) _TypedList buffer -- the underlying data for the view
+ // 2) int offsetInBytes -- the offset into the buffer to start viewing
+ // 3) int length -- the number of elements in the view
+ //
+ // Here, we forward the third.
+ return call->ArgumentAt(3);
}
} else if (CreateArrayInstr* create_array = array->AsCreateArray()) {
if (slot().kind() == Slot::Kind::kArray_length) {
@@ -2719,6 +2753,24 @@
}
}
}
+ } else if (slot().kind() == Slot::Kind::kTypedDataView_data) {
+ // This case cover the first explicit argument to typed data view
+ // factories, the data (buffer).
+ Definition* array = instance()->definition()->OriginalDefinition();
+ if (StaticCallInstr* call = array->AsStaticCall()) {
+ if (IsTypedDataViewFactory(call->function())) {
+ return call->ArgumentAt(1);
+ }
+ }
+ } else if (slot().kind() == Slot::Kind::kTypedDataView_offset_in_bytes) {
+ // This case cover the second explicit argument to typed data view
+ // factories, the offset into the buffer.
+ Definition* array = instance()->definition()->OriginalDefinition();
+ if (StaticCallInstr* call = array->AsStaticCall()) {
+ if (IsTypedDataViewFactory(call->function())) {
+ return call->ArgumentAt(2);
+ }
+ }
} else if (slot().IsTypeArguments()) {
Definition* array = instance()->definition()->OriginalDefinition();
if (StaticCallInstr* call = array->AsStaticCall()) {
diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h
index b1b9d4c..5450f5f 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -5303,6 +5303,7 @@
virtual Definition* Canonicalize(FlowGraph* flow_graph);
static bool IsFixedLengthArrayCid(intptr_t cid);
+ static bool IsTypedDataViewFactory(const Function& function);
virtual bool AllowsCSE() const { return slot_.is_immutable(); }
virtual bool HasUnknownSideEffects() const { return false; }
diff --git a/runtime/vm/compiler/backend/redundancy_elimination_test.cc b/runtime/vm/compiler/backend/redundancy_elimination_test.cc
index e06aca3..e33455e 100644
--- a/runtime/vm/compiler/backend/redundancy_elimination_test.cc
+++ b/runtime/vm/compiler/backend/redundancy_elimination_test.cc
@@ -551,4 +551,170 @@
/* make_host_escape= */ true, MakeAssertAssignable);
}
+// This test verifies behavior of load forwarding when an alias for an
+// allocation A is created after forwarded due to an eliminated load. That is,
+// allocation A is stored and later retrieved via load B, B is used in store C
+// (with a different constant index/index_scale than in B but that overlaps),
+// and then A is retrieved again (with the same index as in B) in load D.
+//
+// When B gets eliminated and replaced with in C and D with A, the store in C
+// should stop the load D from being eliminated. This is a scenario that came
+// up when forwarding typed data view factory arguments.
+//
+// Here, the entire scenario happens within a single basic block.
+ISOLATE_UNIT_TEST_CASE(LoadOptimizer_AliasingViaLoadElimination_SingleBlock) {
+ const char* kScript = R"(
+ import 'dart:typed_data';
+
+ testViewAliasing1() {
+ final f64 = new Float64List(1);
+ final f32 = new Float32List.view(f64.buffer);
+ f64[0] = 1.0; // Should not be forwarded.
+ f32[1] = 2.0; // upper 32bits for 2.0f and 2.0 are the same
+ return f64[0];
+ }
+ )";
+
+ const auto& root_library = Library::Handle(LoadTestScript(kScript));
+ const auto& function =
+ Function::Handle(GetFunction(root_library, "testViewAliasing1"));
+
+ Invoke(root_library, "testViewAliasing1");
+
+ TestPipeline pipeline(function, CompilerPass::kJIT);
+ FlowGraph* flow_graph = pipeline.RunPasses({});
+
+ auto entry = flow_graph->graph_entry()->normal_entry();
+ EXPECT(entry != nullptr);
+
+ StaticCallInstr* list_factory = nullptr;
+ UnboxedConstantInstr* double_one = nullptr;
+ StoreIndexedInstr* first_store = nullptr;
+ StoreIndexedInstr* second_store = nullptr;
+ LoadIndexedInstr* final_load = nullptr;
+ BoxInstr* boxed_result = nullptr;
+
+ ILMatcher cursor(flow_graph, entry);
+ RELEASE_ASSERT(cursor.TryMatch({
+ kMoveGlob,
+ {kMatchAndMoveStaticCall, &list_factory},
+ kMoveGlob,
+ {kMatchAndMoveUnboxedConstant, &double_one},
+ kMoveGlob,
+ {kMatchAndMoveStoreIndexed, &first_store},
+ kMoveGlob,
+ {kMatchAndMoveStoreIndexed, &second_store},
+ kMoveGlob,
+ {kMatchAndMoveLoadIndexed, &final_load},
+ kMoveGlob,
+ {kMatchAndMoveBox, &boxed_result},
+ kMoveGlob,
+ kMatchReturn,
+ }));
+
+ EXPECT(first_store->array()->definition() == list_factory);
+ EXPECT(second_store->array()->definition() == list_factory);
+ EXPECT(boxed_result->value()->definition() != double_one);
+ EXPECT(boxed_result->value()->definition() == final_load);
+}
+
+// This test verifies behavior of load forwarding when an alias for an
+// allocation A is created after forwarded due to an eliminated load. That is,
+// allocation A is stored and later retrieved via load B, B is used in store C
+// (with a different constant index/index_scale than in B but that overlaps),
+// and then A is retrieved again (with the same index as in B) in load D.
+//
+// When B gets eliminated and replaced with in C and D with A, the store in C
+// should stop the load D from being eliminated. This is a scenario that came
+// up when forwarding typed data view factory arguments.
+//
+// Here, the scenario is split across basic blocks. This is a cut-down version
+// of language_2/vm/load_to_load_forwarding_vm_test.dart with just enough extra
+// to keep testViewAliasing1 from being optimized into a single basic block.
+// Thus, this test may be brittler than the other, if future work causes it to
+// end up compiled into a single basic block (or a simpler set of basic blocks).
+ISOLATE_UNIT_TEST_CASE(LoadOptimizer_AliasingViaLoadElimination_AcrossBlocks) {
+ const char* kScript = R"(
+ import 'dart:typed_data';
+
+ class Expect {
+ static void equals(var a, var b) {}
+ static void listEquals(var a, var b) {}
+ }
+
+ testViewAliasing1() {
+ final f64 = new Float64List(1);
+ final f32 = new Float32List.view(f64.buffer);
+ f64[0] = 1.0; // Should not be forwarded.
+ f32[1] = 2.0; // upper 32bits for 2.0f and 2.0 are the same
+ return f64[0];
+ }
+
+ testViewAliasing2() {
+ final f64 = new Float64List(2);
+ final f64v = new Float64List.view(f64.buffer,
+ Float64List.bytesPerElement);
+ f64[1] = 1.0; // Should not be forwarded.
+ f64v[0] = 2.0;
+ return f64[1];
+ }
+
+ testViewAliasing3() {
+ final u8 = new Uint8List(Float64List.bytesPerElement * 2);
+ final f64 = new Float64List.view(u8.buffer, Float64List.bytesPerElement);
+ f64[0] = 1.0; // Should not be forwarded.
+ u8[15] = 0x40;
+ u8[14] = 0x00;
+ return f64[0];
+ }
+
+ main() {
+ for (var i = 0; i < 20; i++) {
+ Expect.equals(2.0, testViewAliasing1());
+ Expect.equals(2.0, testViewAliasing2());
+ Expect.equals(2.0, testViewAliasing3());
+ }
+ }
+ )";
+
+ const auto& root_library = Library::Handle(LoadTestScript(kScript));
+ const auto& function =
+ Function::Handle(GetFunction(root_library, "testViewAliasing1"));
+
+ Invoke(root_library, "main");
+
+ TestPipeline pipeline(function, CompilerPass::kJIT);
+ FlowGraph* flow_graph = pipeline.RunPasses({});
+
+ auto entry = flow_graph->graph_entry()->normal_entry();
+ EXPECT(entry != nullptr);
+
+ StaticCallInstr* list_factory = nullptr;
+ UnboxedConstantInstr* double_one = nullptr;
+ StoreIndexedInstr* first_store = nullptr;
+ StoreIndexedInstr* second_store = nullptr;
+ LoadIndexedInstr* final_load = nullptr;
+ BoxInstr* boxed_result = nullptr;
+
+ ILMatcher cursor(flow_graph, entry);
+ RELEASE_ASSERT(cursor.TryMatch({
+ kMoveGlob, {kMatchAndMoveStaticCall, &list_factory},
+ kMoveGlob, kMatchAndMoveBranchTrue,
+ kMoveGlob, kMatchAndMoveBranchTrue,
+ kMoveGlob, kMatchAndMoveBranchFalse,
+ kMoveGlob, {kMatchAndMoveUnboxedConstant, &double_one},
+ kMoveGlob, {kMatchAndMoveStoreIndexed, &first_store},
+ kMoveGlob, kMatchAndMoveBranchFalse,
+ kMoveGlob, {kMatchAndMoveStoreIndexed, &second_store},
+ kMoveGlob, {kMatchAndMoveLoadIndexed, &final_load},
+ kMoveGlob, {kMatchAndMoveBox, &boxed_result},
+ kMoveGlob, kMatchReturn,
+ }));
+
+ EXPECT(first_store->array()->definition() == list_factory);
+ EXPECT(second_store->array()->definition() == list_factory);
+ EXPECT(boxed_result->value()->definition() != double_one);
+ EXPECT(boxed_result->value()->definition() == final_load);
+}
+
} // namespace dart
diff --git a/tests/language_2/vm/load_to_load_forwarding_cutdown_vm_test.dart b/tests/language_2/vm/load_to_load_forwarding_cutdown_vm_test.dart
new file mode 100644
index 0000000..ed81342
--- /dev/null
+++ b/tests/language_2/vm/load_to_load_forwarding_cutdown_vm_test.dart
@@ -0,0 +1,27 @@
+// Copyright (c) 2012, 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.
+
+// Test correctness of side effects tracking used by load to load forwarding.
+// In this cutdown version of the load_to_load_forwarding_vm test, the function
+// being compiled ends up in a single basic block, which tests load
+// elimination when generating the initial sets.
+
+// VMOptions=--optimization-counter-threshold=10 --no-background-compilation
+
+import "package:expect/expect.dart";
+import "dart:typed_data";
+
+testViewAliasing1() {
+ final f64 = new Float64List(1);
+ final f32 = new Float32List.view(f64.buffer);
+ f64[0] = 1.0; // Should not be forwarded.
+ f32[1] = 2.0; // upper 32bits for 2.0f and 2.0 are the same
+ return f64[0];
+}
+
+main() {
+ for (var i = 0; i < 20; i++) {
+ Expect.equals(2.0, testViewAliasing1());
+ }
+}
