From eb2d2d346e9506e5fe2c8e1e72a146821192b973 Mon Sep 17 00:00:00 2001 From: Mingyao Yang Date: Thu, 2 Mar 2017 13:26:17 -0800 Subject: [PATCH] Allow store elimination for singleton that's returned Allow store elimination for singleton that's visible after method return or deoptimization. Add additional detection for keeping stores for such singletons at block merge/deoptimization point. Bug: 35745320 Test: m test-art-host Change-Id: I8a75a304491dafaeb689787402afa3d7468e3789 --- compiler/optimizing/load_store_elimination.cc | 80 +++++++++++++++++++-------- test/530-checker-lse/src/Main.java | 67 ++++++++++++++++++++++ 2 files changed, 124 insertions(+), 23 deletions(-) diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc index 46ba04873..48699b33a 100644 --- a/compiler/optimizing/load_store_elimination.cc +++ b/compiler/optimizing/load_store_elimination.cc @@ -69,6 +69,13 @@ class ReferenceInfo : public ArenaObject { return is_singleton_and_not_returned_ && is_singleton_and_not_deopt_visible_; } + // Returns true if reference_ is a singleton and returned to the caller or + // used as an environment local of an HDeoptimize instruction. + bool IsSingletonAndNonRemovable() const { + return is_singleton_ && + (!is_singleton_and_not_returned_ || !is_singleton_and_not_deopt_visible_); + } + bool HasIndexAliasing() { return has_index_aliasing_; } @@ -339,11 +346,7 @@ class HeapLocationCollector : public HGraphVisitor { return false; } ReferenceInfo* ref_info = loc1->GetReferenceInfo(); - if (ref_info->IsSingleton()) { - // This is guaranteed by the CanReferencesAlias() test above. - DCHECK_EQ(ref_info, loc2->GetReferenceInfo()); - ref_info->SetHasIndexAliasing(true); - } + ref_info->SetHasIndexAliasing(true); } return true; } @@ -628,14 +631,17 @@ class LSEVisitor : public HGraphVisitor { for (size_t i = 0; i < heap_values.size(); i++) { HeapLocation* location = heap_location_collector_.GetHeapLocation(i); ReferenceInfo* ref_info = location->GetReferenceInfo(); - if (!ref_info->IsSingleton() || location->IsValueKilledByLoopSideEffects()) { - // heap value is killed by loop side effects (stored into directly, or due to - // aliasing). + if (ref_info->IsSingletonAndRemovable() && + !location->IsValueKilledByLoopSideEffects()) { + // A removable singleton's field that's not stored into inside a loop is + // invariant throughout the loop. Nothing to do. + DCHECK(ref_info->IsSingletonAndRemovable()); + } else { + // heap value is killed by loop side effects (stored into directly, or + // due to aliasing). Or the heap value may be needed after method return + // or deoptimization. KeepIfIsStore(pre_header_heap_values[i]); heap_values[i] = kUnknownHeapValue; - } else { - // A singleton's field that's not stored into inside a loop is invariant throughout - // the loop. } } } @@ -654,7 +660,7 @@ class LSEVisitor : public HGraphVisitor { bool from_all_predecessors = true; ReferenceInfo* ref_info = heap_location_collector_.GetHeapLocation(i)->GetReferenceInfo(); HInstruction* singleton_ref = nullptr; - if (ref_info->IsSingletonAndRemovable()) { + if (ref_info->IsSingleton()) { // We do more analysis of liveness when merging heap values for such // cases since stores into such references may potentially be eliminated. singleton_ref = ref_info->GetReference(); @@ -680,8 +686,9 @@ class LSEVisitor : public HGraphVisitor { } } - if (merged_value == kUnknownHeapValue) { - // There are conflicting heap values from different predecessors. + if (merged_value == kUnknownHeapValue || ref_info->IsSingletonAndNonRemovable()) { + // There are conflicting heap values from different predecessors, + // or the heap value may be needed after method return or deoptimization. // Keep the last store in each predecessor since future loads cannot be eliminated. for (HBasicBlock* predecessor : predecessors) { ArenaVector& pred_values = heap_values_for_[predecessor->GetBlockId()]; @@ -828,15 +835,15 @@ class LSEVisitor : public HGraphVisitor { // Store into the heap location with the same value. same_value = true; } else if (index != nullptr && ref_info->HasIndexAliasing()) { - // For array element, don't eliminate stores if the index can be - // aliased. - } else if (ref_info->IsSingletonAndRemovable()) { - // Store into a field/element of a singleton instance/array that's not returned. - // The value cannot be killed due to aliasing/invocation. It can be redundant since - // future loads can directly get the value set by this instruction. The value can - // still be killed due to merging or loop side effects. Stores whose values are - // killed due to merging/loop side effects later will be removed from - // possibly_removed_stores_ when that is detected. + // For array element, don't eliminate stores if the index can be aliased. + } else if (ref_info->IsSingleton()) { + // Store into a field of a singleton. The value cannot be killed due to + // aliasing/invocation. It can be redundant since future loads can + // directly get the value set by this instruction. The value can still be killed due to + // merging or loop side effects. Stores whose values are killed due to merging/loop side + // effects later will be removed from possibly_removed_stores_ when that is detected. + // Stores whose values may be needed after method return or deoptimization + // are also removed from possibly_removed_stores_ when that is detected. possibly_redundant = true; HNewInstance* new_instance = ref_info->GetReference()->AsNewInstance(); if (new_instance != nullptr && new_instance->IsFinalizable()) { @@ -945,6 +952,33 @@ class LSEVisitor : public HGraphVisitor { value); } + void VisitDeoptimize(HDeoptimize* instruction) { + const ArenaVector& heap_values = + heap_values_for_[instruction->GetBlock()->GetBlockId()]; + for (HInstruction* heap_value : heap_values) { + // Filter out fake instructions before checking instruction kind below. + if (heap_value == kUnknownHeapValue || heap_value == kDefaultHeapValue) { + continue; + } + // A store is kept as the heap value for possibly removed stores. + if (heap_value->IsInstanceFieldSet() || heap_value->IsArraySet()) { + // Check whether the reference for a store is used by an environment local of + // HDeoptimize. + HInstruction* reference = heap_value->InputAt(0); + DCHECK(heap_location_collector_.FindReferenceInfoOf(reference)->IsSingleton()); + for (const HUseListNode& use : reference->GetEnvUses()) { + HEnvironment* user = use.GetUser(); + if (user->GetHolder() == instruction) { + // The singleton for the store is visible at this deoptimization + // point. Need to keep the store so that the heap value is + // seen by the interpreter. + KeepIfIsStore(heap_value); + } + } + } + } + } + void HandleInvoke(HInstruction* invoke) { ArenaVector& heap_values = heap_values_for_[invoke->GetBlock()->GetBlockId()]; diff --git a/test/530-checker-lse/src/Main.java b/test/530-checker-lse/src/Main.java index 14a40ef31..663250369 100644 --- a/test/530-checker-lse/src/Main.java +++ b/test/530-checker-lse/src/Main.java @@ -747,6 +747,69 @@ public class Main { return 1.0f; } + /// CHECK-START: TestClass2 Main.testStoreStore() load_store_elimination (before) + /// CHECK: NewInstance + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + + /// CHECK-START: TestClass2 Main.testStoreStore() load_store_elimination (after) + /// CHECK: NewInstance + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK-NOT: InstanceFieldSet + + private static TestClass2 testStoreStore() { + TestClass2 obj = new TestClass2(); + obj.i = 41; + obj.j = 42; + obj.i = 41; + obj.j = 43; + return obj; + } + + /// CHECK-START: int Main.testStoreStoreWithDeoptimize(int[]) load_store_elimination (before) + /// CHECK: NewInstance + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK: Deoptimize + /// CHECK: ArraySet + /// CHECK: ArraySet + /// CHECK: ArraySet + /// CHECK: ArraySet + /// CHECK: ArrayGet + /// CHECK: ArrayGet + /// CHECK: ArrayGet + /// CHECK: ArrayGet + + /// CHECK-START: int Main.testStoreStoreWithDeoptimize(int[]) load_store_elimination (after) + /// CHECK: NewInstance + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldSet + /// CHECK-NOT: InstanceFieldSet + /// CHECK: Deoptimize + /// CHECK: ArraySet + /// CHECK: ArraySet + /// CHECK: ArraySet + /// CHECK: ArraySet + /// CHECK-NOT: ArrayGet + + private static int testStoreStoreWithDeoptimize(int[] arr) { + TestClass2 obj = new TestClass2(); + obj.i = 41; + obj.j = 42; + obj.i = 41; + obj.j = 43; + arr[0] = 1; // One HDeoptimize here. + arr[1] = 1; + arr[2] = 1; + arr[3] = 1; + return arr[0] + arr[1] + arr[2] + arr[3]; + } + /// CHECK-START: double Main.getCircleArea(double, boolean) load_store_elimination (before) /// CHECK: NewInstance @@ -950,6 +1013,10 @@ public class Main { assertIntEquals(testAllocationEliminationOfArray2(), 11); assertIntEquals(testAllocationEliminationOfArray3(2), 4); assertIntEquals(testAllocationEliminationOfArray4(2), 6); + + assertIntEquals(testStoreStore().i, 41); + assertIntEquals(testStoreStore().j, 43); + assertIntEquals(testStoreStoreWithDeoptimize(new int[4]), 4); } static boolean sFlag; -- 2.11.0