LSE fix removing observable stores in try catch
Follow-up to aosp/2072271, focusing on try catches.
Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Bug: 229706824, 227283233
Change-Id: Ic935a6d9f1b135b2ec9d28ca6b64df2001c0aa19
(cherry picked from commit 614638b9c178552953feda9ed366bd877002f1c9)
Merged-In: Ic935a6d9f1b135b2ec9d28ca6b64df2001c0aa19
diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc
index e3d8481..9b8f07e 100644
--- a/compiler/optimizing/load_store_elimination.cc
+++ b/compiler/optimizing/load_store_elimination.cc
@@ -1040,6 +1040,8 @@
}
void VisitDeoptimize(HDeoptimize* instruction) override {
+ // If we are in a try catch, even singletons are observable.
+ const bool in_try_catch = instruction->GetBlock()->GetTryCatchInformation() != nullptr;
HBasicBlock* block = instruction->GetBlock();
ScopedArenaVector<ValueRecord>& heap_values = heap_values_for_[block->GetBlockId()];
for (size_t i = 0u, size = heap_values.size(); i != size; ++i) {
@@ -1051,7 +1053,7 @@
// for singletons that don't escape in the deoptimization environment.
bool observable = true;
ReferenceInfo* info = heap_location_collector_.GetHeapLocation(i)->GetReferenceInfo();
- if (info->IsSingleton()) {
+ if (!in_try_catch && info->IsSingleton()) {
HInstruction* reference = info->GetReference();
// Finalizable objects always escape.
const bool finalizable_object =
@@ -1076,11 +1078,11 @@
}
// Keep necessary stores before exiting a method via return/throw.
- void HandleExit(HBasicBlock* block) {
+ void HandleExit(HBasicBlock* block, bool must_keep_stores = false) {
ScopedArenaVector<ValueRecord>& heap_values = heap_values_for_[block->GetBlockId()];
for (size_t i = 0u, size = heap_values.size(); i != size; ++i) {
ReferenceInfo* ref_info = heap_location_collector_.GetHeapLocation(i)->GetReferenceInfo();
- if (IsEscapingObject(ref_info, block, i)) {
+ if (must_keep_stores || IsEscapingObject(ref_info, block, i)) {
KeepStores(heap_values[i].stored_by);
heap_values[i].stored_by = Value::PureUnknown();
}
@@ -1097,7 +1099,10 @@
void HandleThrowingInstruction(HInstruction* instruction) {
DCHECK(instruction->CanThrow());
- HandleExit(instruction->GetBlock());
+ // If we are inside of a try catch, singletons can become visible since we may not exit the
+ // method.
+ HandleExit(instruction->GetBlock(),
+ instruction->GetBlock()->GetTryCatchInformation() != nullptr);
}
void VisitMethodEntryHook(HMethodEntryHook* method_entry) override {
@@ -1153,6 +1158,9 @@
void HandleInvoke(HInstruction* instruction) {
// If `instruction` can throw we have to presume all stores are visible.
const bool can_throw = instruction->CanThrow();
+ // If we are in a try catch, even singletons are observable.
+ const bool can_throw_in_try_catch =
+ can_throw && instruction->GetBlock()->GetTryCatchInformation() != nullptr;
SideEffects side_effects = instruction->GetSideEffects();
ScopedArenaVector<ValueRecord>& heap_values =
heap_values_for_[instruction->GetBlock()->GetBlockId()];
@@ -1178,9 +1186,10 @@
return cohort.PrecedesBlock(blk);
});
};
- if (ref_info->IsSingleton() ||
- // partial and we aren't currently escaping and we haven't escaped yet.
- (ref_info->IsPartialSingleton() && partial_singleton_did_not_escape(ref_info, blk))) {
+ if (!can_throw_in_try_catch &&
+ (ref_info->IsSingleton() ||
+ // partial and we aren't currently escaping and we haven't escaped yet.
+ (ref_info->IsPartialSingleton() && partial_singleton_did_not_escape(ref_info, blk)))) {
// Singleton references cannot be seen by the callee.
} else {
if (can_throw || side_effects.DoesAnyRead() || side_effects.DoesAnyWrite()) {
@@ -1226,6 +1235,8 @@
}
void VisitNewInstance(HNewInstance* new_instance) override {
+ // If we are in a try catch, even singletons are observable.
+ const bool in_try_catch = new_instance->GetBlock()->GetTryCatchInformation() != nullptr;
ReferenceInfo* ref_info = heap_location_collector_.FindReferenceInfoOf(new_instance);
if (ref_info == nullptr) {
// new_instance isn't used for field accesses. No need to process it.
@@ -1254,7 +1265,7 @@
heap_values[i].value = Value::ForInstruction(new_instance->GetLoadClass());
heap_values[i].stored_by = Value::PureUnknown();
}
- } else if (IsEscapingObject(info, block, i)) {
+ } else if (in_try_catch || IsEscapingObject(info, block, i)) {
// Since NewInstance can throw, we presume all previous stores could be visible.
KeepStores(heap_values[i].stored_by);
heap_values[i].stored_by = Value::PureUnknown();
@@ -1263,6 +1274,8 @@
}
void VisitNewArray(HNewArray* new_array) override {
+ // If we are in a try catch, even singletons are observable.
+ const bool in_try_catch = new_array->GetBlock()->GetTryCatchInformation() != nullptr;
ReferenceInfo* ref_info = heap_location_collector_.FindReferenceInfoOf(new_array);
if (ref_info == nullptr) {
// new_array isn't used for array accesses. No need to process it.
@@ -1287,7 +1300,7 @@
// Array elements are set to default heap values.
heap_values[i].value = Value::Default();
heap_values[i].stored_by = Value::PureUnknown();
- } else if (IsEscapingObject(info, block, i)) {
+ } else if (in_try_catch || IsEscapingObject(info, block, i)) {
// Since NewArray can throw, we presume all previous stores could be visible.
KeepStores(heap_values[i].stored_by);
heap_values[i].stored_by = Value::PureUnknown();
diff --git a/test/2042-checker-dce-always-throw/Android.bp b/test/2042-checker-dce-always-throw/Android.bp
new file mode 100644
index 0000000..f991939
--- /dev/null
+++ b/test/2042-checker-dce-always-throw/Android.bp
@@ -0,0 +1,43 @@
+// Generated by `regen-test-files`. Do not edit manually.
+
+// Build rules for ART run-test `2042-checker-dce-always-throw`.
+
+package {
+ // See: http://go/android-license-faq
+ // A large-scale-change added 'default_applicable_licenses' to import
+ // all of the 'license_kinds' from "art_license"
+ // to get the below license kinds:
+ // SPDX-license-identifier-Apache-2.0
+ default_applicable_licenses: ["art_license"],
+}
+
+// Test's Dex code.
+java_test {
+ name: "art-run-test-2042-checker-dce-always-throw",
+ defaults: ["art-run-test-defaults"],
+ test_config_template: ":art-run-test-target-template",
+ srcs: ["src/**/*.java"],
+ data: [
+ ":art-run-test-2042-checker-dce-always-throw-expected-stdout",
+ ":art-run-test-2042-checker-dce-always-throw-expected-stderr",
+ ],
+ // Include the Java source files in the test's artifacts, to make Checker assertions
+ // available to the TradeFed test runner.
+ include_srcs: true,
+}
+
+// Test's expected standard output.
+genrule {
+ name: "art-run-test-2042-checker-dce-always-throw-expected-stdout",
+ out: ["art-run-test-2042-checker-dce-always-throw-expected-stdout.txt"],
+ srcs: ["expected-stdout.txt"],
+ cmd: "cp -f $(in) $(out)",
+}
+
+// Test's expected standard error.
+genrule {
+ name: "art-run-test-2042-checker-dce-always-throw-expected-stderr",
+ out: ["art-run-test-2042-checker-dce-always-throw-expected-stderr.txt"],
+ srcs: ["expected-stderr.txt"],
+ cmd: "cp -f $(in) $(out)",
+}
diff --git a/test/530-checker-loops-try-catch/Android.bp b/test/530-checker-loops-try-catch/Android.bp
new file mode 100644
index 0000000..b7b4127
--- /dev/null
+++ b/test/530-checker-loops-try-catch/Android.bp
@@ -0,0 +1,43 @@
+// Generated by `regen-test-files`. Do not edit manually.
+
+// Build rules for ART run-test `530-checker-loops-try-catch`.
+
+package {
+ // See: http://go/android-license-faq
+ // A large-scale-change added 'default_applicable_licenses' to import
+ // all of the 'license_kinds' from "art_license"
+ // to get the below license kinds:
+ // SPDX-license-identifier-Apache-2.0
+ default_applicable_licenses: ["art_license"],
+}
+
+// Test's Dex code.
+java_test {
+ name: "art-run-test-530-checker-loops-try-catch",
+ defaults: ["art-run-test-defaults"],
+ test_config_template: ":art-run-test-target-template",
+ srcs: ["src/**/*.java"],
+ data: [
+ ":art-run-test-530-checker-loops-try-catch-expected-stdout",
+ ":art-run-test-530-checker-loops-try-catch-expected-stderr",
+ ],
+ // Include the Java source files in the test's artifacts, to make Checker assertions
+ // available to the TradeFed test runner.
+ include_srcs: true,
+}
+
+// Test's expected standard output.
+genrule {
+ name: "art-run-test-530-checker-loops-try-catch-expected-stdout",
+ out: ["art-run-test-530-checker-loops-try-catch-expected-stdout.txt"],
+ srcs: ["expected-stdout.txt"],
+ cmd: "cp -f $(in) $(out)",
+}
+
+// Test's expected standard error.
+genrule {
+ name: "art-run-test-530-checker-loops-try-catch-expected-stderr",
+ out: ["art-run-test-530-checker-loops-try-catch-expected-stderr.txt"],
+ srcs: ["expected-stderr.txt"],
+ cmd: "cp -f $(in) $(out)",
+}
diff --git a/test/530-checker-lse-try-catch/Android.bp b/test/530-checker-lse-try-catch/Android.bp
new file mode 100644
index 0000000..6a58ea7
--- /dev/null
+++ b/test/530-checker-lse-try-catch/Android.bp
@@ -0,0 +1,43 @@
+// Generated by `regen-test-files`. Do not edit manually.
+
+// Build rules for ART run-test `530-checker-lse-try-catch`.
+
+package {
+ // See: http://go/android-license-faq
+ // A large-scale-change added 'default_applicable_licenses' to import
+ // all of the 'license_kinds' from "art_license"
+ // to get the below license kinds:
+ // SPDX-license-identifier-Apache-2.0
+ default_applicable_licenses: ["art_license"],
+}
+
+// Test's Dex code.
+java_test {
+ name: "art-run-test-530-checker-lse-try-catch",
+ defaults: ["art-run-test-defaults"],
+ test_config_template: ":art-run-test-target-template",
+ srcs: ["src/**/*.java"],
+ data: [
+ ":art-run-test-530-checker-lse-try-catch-expected-stdout",
+ ":art-run-test-530-checker-lse-try-catch-expected-stderr",
+ ],
+ // Include the Java source files in the test's artifacts, to make Checker assertions
+ // available to the TradeFed test runner.
+ include_srcs: true,
+}
+
+// Test's expected standard output.
+genrule {
+ name: "art-run-test-530-checker-lse-try-catch-expected-stdout",
+ out: ["art-run-test-530-checker-lse-try-catch-expected-stdout.txt"],
+ srcs: ["expected-stdout.txt"],
+ cmd: "cp -f $(in) $(out)",
+}
+
+// Test's expected standard error.
+genrule {
+ name: "art-run-test-530-checker-lse-try-catch-expected-stderr",
+ out: ["art-run-test-530-checker-lse-try-catch-expected-stderr.txt"],
+ srcs: ["expected-stderr.txt"],
+ cmd: "cp -f $(in) $(out)",
+}
diff --git a/test/530-checker-lse-try-catch/src/Main.java b/test/530-checker-lse-try-catch/src/Main.java
index c7db02a..56ef8bd 100644
--- a/test/530-checker-lse-try-catch/src/Main.java
+++ b/test/530-checker-lse-try-catch/src/Main.java
@@ -30,6 +30,8 @@
assertEquals(1, $noinline$testTryCatchBlocking(new Point(), boolean_throw));
assertEquals(1, $noinline$testTryCatchPhi(new Point(), boolean_throw));
assertEquals(2, $noinline$testTryCatchPhiWithTwoCatches(new Point(), new int[0]));
+ assertEquals(1, $noinline$testKeepStoreInsideTryCatch());
+ assertEquals(150, $noinline$test40());
}
/// CHECK-START: int Main.$noinline$testDifferentFields(Point, Point, boolean, boolean) load_store_elimination (before)
@@ -199,9 +201,113 @@
return obj.y;
}
+ // Check that we don't eliminate the first store to `main.sumForKeepStoreInsideTryCatch` since it
+ // is observable.
+
+ // Consistency check to make sure the try/catch wasn't removed by an earlier pass.
+ /// CHECK-START: int Main.$noinline$testKeepStoreInsideTryCatch() load_store_elimination (after)
+ /// CHECK-DAG: TryBoundary kind:entry
+
+ /// CHECK-START: int Main.$noinline$testKeepStoreInsideTryCatch() load_store_elimination (before)
+ /// CHECK: InstanceFieldSet field_name:Main.sumForKeepStoreInsideTryCatch
+ /// CHECK: InstanceFieldSet field_name:Main.sumForKeepStoreInsideTryCatch
+
+ /// CHECK-START: int Main.$noinline$testKeepStoreInsideTryCatch() load_store_elimination (after)
+ /// CHECK: InstanceFieldSet field_name:Main.sumForKeepStoreInsideTryCatch
+ /// CHECK: InstanceFieldSet field_name:Main.sumForKeepStoreInsideTryCatch
+ private static int $noinline$testKeepStoreInsideTryCatch() {
+ Main main = new Main();
+ main.sumForKeepStoreInsideTryCatch = 0;
+ try {
+ int[] array = {1};
+ main.sumForKeepStoreInsideTryCatch += array[0];
+ main.sumForKeepStoreInsideTryCatch += array[1];
+ throw new RuntimeException("Unreachable");
+ } catch (ArrayIndexOutOfBoundsException e) {
+ return main.sumForKeepStoreInsideTryCatch;
+ }
+ }
+
+ /// CHECK-START: int Main.$noinline$test40() load_store_elimination (before)
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ /// CHECK: ArraySet
+ //
+ /// CHECK: ArraySet
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ //
+ /// CHECK: ArraySet
+ /// CHECK: ArraySet
+ /// CHECK: ArraySet
+ /// CHECK-NOT: ArraySet
+
+ /// CHECK-START: int Main.$noinline$test40() load_store_elimination (after)
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ //
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ //
+ /// CHECK-NOT: ArraySet
+
+ // Like `test40` from 530-checker-lse but with $inline$ for the inner method so we check that we
+ // have the array set inside try catches too.
+ // Since we are inlining, we know the parameters and are able to elimnate (some) of the
+ // `ArraySet`s.
+ private static int $noinline$test40() {
+ int[] array = new int[1];
+ try {
+ $inline$fillArrayTest40(array, 100, 0);
+ System.out.println("UNREACHABLE");
+ } catch (Throwable expected) {
+ }
+ assertEquals(1, array[0]);
+ try {
+ $inline$fillArrayTest40(array, 100, 1);
+ System.out.println("UNREACHABLE");
+ } catch (Throwable expected) {
+ }
+ assertEquals(2, array[0]);
+ $inline$fillArrayTest40(array, 100, 2);
+ assertEquals(150, array[0]);
+ return array[0];
+ }
+
+ /// CHECK-START: void Main.$inline$fillArrayTest40(int[], int, int) load_store_elimination (before)
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ /// CHECK-NOT: ArraySet
+
+ /// CHECK-START: void Main.$inline$fillArrayTest40(int[], int, int) load_store_elimination (after)
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ /// CHECK: DivZeroCheck
+ /// CHECK: ArraySet
+ /// CHECK-NOT: ArraySet
+
+ // Check that the stores to array[0] are not eliminated since we can throw in between the stores.
+ private static void $inline$fillArrayTest40(int[] array, int a, int b) {
+ array[0] = 1;
+ int x = a / b;
+ array[0] = 2;
+ int y = a / (b - 1);
+ array[0] = x + y;
+ }
+
private static void assertEquals(int expected, int actual) {
if (expected != actual) {
throw new AssertionError("Expected: " + expected + ", Actual: " + actual);
}
}
+
+ int sumForKeepStoreInsideTryCatch;
}