[vm/compiler] Fix incorrect DSE
Dead Store Elimination (DSE) propagates "dead store" state backwards
across basic blocks to predecessors. If place is defined in a loop
(either its instance or index are defined in a loop), propagating
"dead store" state across backedge of the loop is incorrect, as each
loop iteration effectively has its own place.
This bug is fixed by disabling propagation of "dead store"
state beyond defining blocks of instance and index of a place.
This is done by setting bits in "live_in" of the blocks corresponding
to the instance and index definitions of a place.
TEST=runtime/tests/vm/dart/regress_55607_test.dart
Fixes https://github.com/dart-lang/sdk/issues/55607
Change-Id: I223ff433daa3d7bcafefc9d91360b16c9554964e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/365302
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
diff --git a/runtime/tests/vm/dart/regress_55607_test.dart b/runtime/tests/vm/dart/regress_55607_test.dart
new file mode 100644
index 0000000..813e98c
--- /dev/null
+++ b/runtime/tests/vm/dart/regress_55607_test.dart
@@ -0,0 +1,81 @@
+// Copyright (c) 2024, 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.
+
+// Verifies that Dead Store Elimination (DSE) doesn't eliminate
+// stores after incorrectly propagating "dead store" state beyond
+// definition of instance and index of a store via loop backedge.
+// Regression test for https://github.com/dart-lang/sdk/issues/55607.
+
+import 'package:expect/expect.dart';
+
+// Original example.
+// Verifies that "dead store" state is not be propagated for indexed place
+// 'part[offset]' beyond definition of index 'offset' which changes in a loop.
+@pragma('vm:never-inline')
+List<int> test1() {
+ int offset = 0;
+ List<int> part = [0, 0, 0];
+ for (int i = 0; i < 2; i++) {
+ part[offset] = 2;
+ offset++;
+ }
+ part[offset] = 10;
+ return part;
+}
+
+class A {
+ int aField;
+ A(this.aField);
+ operator ==(Object other) => other is A && this.aField == other.aField;
+ String toString() => 'A($aField)';
+}
+
+// Verifies that "dead store" state is not be propagated for instance field
+// place 'obj.aField' beyond definition of instance 'obj' which changes in
+// a loop.
+@pragma('vm:never-inline')
+List<A> test2() {
+ A obj1 = A(0);
+ A obj2 = A(0);
+ final list = [obj1, obj2];
+ A obj = obj1;
+ for (int i = 0; i < 1; i++) {
+ obj.aField = 2;
+ obj = obj2;
+ }
+ obj.aField = 10;
+ return list;
+}
+
+class B {
+ A obj;
+ B(this.obj);
+ operator ==(Object other) => other is B && this.obj == other.obj;
+ String toString() => 'B($obj)';
+}
+
+@pragma('vm:never-inline')
+confuse(x) => x;
+
+// Same as previous, but 'obj' definition is LoadField, not a Phi.
+@pragma('vm:never-inline')
+List<B> test3() {
+ B b1 = confuse(B(A(0)));
+ B b2 = confuse(B(A(0)));
+ final list = [b1, b2];
+ A obj;
+ B bb = b1;
+ for (int i = 0; (obj = bb.obj) != null && i < 1; i++) {
+ obj.aField = 2;
+ bb = b2;
+ }
+ obj.aField = 10;
+ return list;
+}
+
+void main() {
+ Expect.deepEquals([2, 2, 10], test1());
+ Expect.deepEquals([A(2), A(10)], test2());
+ Expect.deepEquals([B(A(2)), B(A(10))], test3());
+}
diff --git a/runtime/vm/compiler/backend/redundancy_elimination.cc b/runtime/vm/compiler/backend/redundancy_elimination.cc
index b72d1ed..e5bd8c3 100644
--- a/runtime/vm/compiler/backend/redundancy_elimination.cc
+++ b/runtime/vm/compiler/backend/redundancy_elimination.cc
@@ -3080,24 +3080,43 @@
new (zone) BitVector(zone, aliased_set_->max_place_id());
all_places->SetAll();
- BitVector* all_aliased_places =
- new (zone) BitVector(zone, aliased_set_->max_place_id());
+ BitVector* all_aliased_places = nullptr;
if (CompilerState::Current().is_aot()) {
- const auto& places = aliased_set_->places();
- // Go through all places and identify those which are escaping.
- // We find such places by inspecting definition allocation
- // [AliasIdentity] field, which is populated above by
- // [AliasedSet::ComputeAliasing].
- for (intptr_t i = 0; i < places.length(); i++) {
- Place* place = places[i];
- if (place->DependsOnInstance()) {
- Definition* instance = place->instance();
- if (Place::IsAllocation(instance) &&
- !instance->Identity().IsAliased()) {
- continue;
- }
+ all_aliased_places =
+ new (zone) BitVector(zone, aliased_set_->max_place_id());
+ }
+ const auto& places = aliased_set_->places();
+ for (intptr_t i = 0; i < places.length(); i++) {
+ Place* place = places[i];
+ if (place->DependsOnInstance()) {
+ Definition* instance = place->instance();
+ // Find escaping places by inspecting definition allocation
+ // [AliasIdentity] field, which is populated above by
+ // [AliasedSet::ComputeAliasing].
+ if ((all_aliased_places != nullptr) &&
+ (!Place::IsAllocation(instance) ||
+ instance->Identity().IsAliased())) {
+ all_aliased_places->Add(i);
}
- all_aliased_places->Add(i);
+ if (instance != nullptr) {
+ // Avoid incorrect propagation of "dead" state beyond definition
+ // of the instance. Otherwise it may eventually reach stores into
+ // this place via loop backedge.
+ live_in_[instance->GetBlock()->postorder_number()]->Add(i);
+ }
+ } else {
+ if (all_aliased_places != nullptr) {
+ all_aliased_places->Add(i);
+ }
+ }
+ if (place->kind() == Place::kIndexed) {
+ Definition* index = place->index();
+ if (index != nullptr) {
+ // Avoid incorrect propagation of "dead" state beyond definition
+ // of the index. Otherwise it may eventually reach stores into
+ // this place via loop backedge.
+ live_in_[index->GetBlock()->postorder_number()]->Add(i);
+ }
}
}
