Revert "LSE fix to allow different operation kinds in the same heap location"
This reverts commit 717ffa8fd155e591e16fa4ac1cf4357f974450da.
Reason for revert: Some failures due to CHECKer pattern matching e.g. https://ci.chromium.org/ui/p/art/builders/ci/angler-armv7-ndebug/3635/overview and art-heap-poisoning
Change-Id: Ia00a5b81e9e9539a9cc140937ea1103f251345e9
diff --git a/compiler/optimizing/load_store_analysis.h b/compiler/optimizing/load_store_analysis.h
index c46a5b9..c0799db 100644
--- a/compiler/optimizing/load_store_analysis.h
+++ b/compiler/optimizing/load_store_analysis.h
@@ -171,16 +171,14 @@
size_t offset,
HInstruction* index,
size_t vector_length,
- int16_t declaring_class_def_index,
- bool is_vec_op)
+ int16_t declaring_class_def_index)
: ref_info_(ref_info),
type_(DataType::ToSigned(type)),
offset_(offset),
index_(index),
vector_length_(vector_length),
declaring_class_def_index_(declaring_class_def_index),
- has_aliased_locations_(false),
- is_vec_op_(is_vec_op) {
+ has_aliased_locations_(false) {
DCHECK(ref_info != nullptr);
DCHECK((offset == kInvalidFieldOffset && index != nullptr) ||
(offset != kInvalidFieldOffset && index == nullptr));
@@ -191,7 +189,6 @@
size_t GetOffset() const { return offset_; }
HInstruction* GetIndex() const { return index_; }
size_t GetVectorLength() const { return vector_length_; }
- bool IsVecOp() const { return is_vec_op_; }
// Returns the definition of declaring class' dex index.
// It's kDeclaringClassDefIndexForArrays for an array element.
@@ -230,12 +227,11 @@
// Declaring class's def's dex index.
// Invalid when this HeapLocation is not field access.
const int16_t declaring_class_def_index_;
+
// Has aliased heap locations in the method, due to either the
// reference is aliased or the array element is aliased via different
// index names.
bool has_aliased_locations_;
- // Whether this HeapLocation represents a vector operation.
- bool is_vec_op_;
DISALLOW_COPY_AND_ASSIGN(HeapLocation);
};
@@ -322,8 +318,7 @@
field->GetFieldOffset().SizeValue(),
nullptr,
HeapLocation::kScalar,
- field->GetDeclaringClassDefIndex(),
- /*is_vec_op=*/false);
+ field->GetDeclaringClassDefIndex());
}
size_t GetArrayHeapLocation(HInstruction* instruction) const {
@@ -332,10 +327,10 @@
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetType();
size_t vector_length = HeapLocation::kScalar;
- const bool is_vec_op = instruction->IsVecStore() || instruction->IsVecLoad();
if (instruction->IsArraySet()) {
type = instruction->AsArraySet()->GetComponentType();
- } else if (is_vec_op) {
+ } else if (instruction->IsVecStore() ||
+ instruction->IsVecLoad()) {
HVecOperation* vec_op = instruction->AsVecOperation();
type = vec_op->GetPackedType();
vector_length = vec_op->GetVectorLength();
@@ -347,8 +342,7 @@
HeapLocation::kInvalidFieldOffset,
index,
vector_length,
- HeapLocation::kDeclaringClassDefIndexForArrays,
- is_vec_op);
+ HeapLocation::kDeclaringClassDefIndexForArrays);
}
bool HasHeapStores() const {
@@ -370,8 +364,7 @@
size_t offset,
HInstruction* index,
size_t vector_length,
- int16_t declaring_class_def_index,
- bool is_vec_op) const {
+ int16_t declaring_class_def_index) const {
DataType::Type lookup_type = DataType::ToSigned(type);
for (size_t i = 0; i < heap_locations_.size(); i++) {
HeapLocation* loc = heap_locations_[i];
@@ -380,8 +373,7 @@
loc->GetOffset() == offset &&
loc->GetIndex() == index &&
loc->GetVectorLength() == vector_length &&
- loc->GetDeclaringClassDefIndex() == declaring_class_def_index &&
- loc->IsVecOp() == is_vec_op) {
+ loc->GetDeclaringClassDefIndex() == declaring_class_def_index) {
return i;
}
}
@@ -526,15 +518,14 @@
size_t offset,
HInstruction* index,
size_t vector_length,
- int16_t declaring_class_def_index,
- bool is_vec_op) {
+ int16_t declaring_class_def_index) {
HInstruction* original_ref = HuntForOriginalReference(ref);
ReferenceInfo* ref_info = GetOrCreateReferenceInfo(original_ref);
size_t heap_location_idx = FindHeapLocationIndex(
- ref_info, type, offset, index, vector_length, declaring_class_def_index, is_vec_op);
+ ref_info, type, offset, index, vector_length, declaring_class_def_index);
if (heap_location_idx == kHeapLocationNotFound) {
- HeapLocation* heap_loc = new (allocator_) HeapLocation(
- ref_info, type, offset, index, vector_length, declaring_class_def_index, is_vec_op);
+ HeapLocation* heap_loc = new (allocator_)
+ HeapLocation(ref_info, type, offset, index, vector_length, declaring_class_def_index);
heap_locations_.push_back(heap_loc);
}
}
@@ -548,22 +539,19 @@
offset,
nullptr,
HeapLocation::kScalar,
- declaring_class_def_index,
- /*is_vec_op=*/false);
+ declaring_class_def_index);
}
void VisitArrayAccess(HInstruction* array,
HInstruction* index,
DataType::Type type,
- size_t vector_length,
- bool is_vec_op) {
+ size_t vector_length) {
MaybeCreateHeapLocation(array,
type,
HeapLocation::kInvalidFieldOffset,
index,
vector_length,
- HeapLocation::kDeclaringClassDefIndexForArrays,
- is_vec_op);
+ HeapLocation::kDeclaringClassDefIndexForArrays);
}
void VisitPredicatedInstanceFieldGet(HPredicatedInstanceFieldGet* instruction) override {
@@ -597,7 +585,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetType();
- VisitArrayAccess(array, index, type, HeapLocation::kScalar, /*is_vec_op=*/false);
+ VisitArrayAccess(array, index, type, HeapLocation::kScalar);
CreateReferenceInfoForReferenceType(instruction);
}
@@ -605,7 +593,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetComponentType();
- VisitArrayAccess(array, index, type, HeapLocation::kScalar, /*is_vec_op=*/false);
+ VisitArrayAccess(array, index, type, HeapLocation::kScalar);
has_heap_stores_ = true;
}
@@ -613,7 +601,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetPackedType();
- VisitArrayAccess(array, index, type, instruction->GetVectorLength(), /*is_vec_op=*/true);
+ VisitArrayAccess(array, index, type, instruction->GetVectorLength());
CreateReferenceInfoForReferenceType(instruction);
}
@@ -621,7 +609,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetPackedType();
- VisitArrayAccess(array, index, type, instruction->GetVectorLength(), /*is_vec_op=*/true);
+ VisitArrayAccess(array, index, type, instruction->GetVectorLength());
has_heap_stores_ = true;
}
diff --git a/compiler/optimizing/load_store_analysis_test.cc b/compiler/optimizing/load_store_analysis_test.cc
index b6cf8b4..7930c0d 100644
--- a/compiler/optimizing/load_store_analysis_test.cc
+++ b/compiler/optimizing/load_store_analysis_test.cc
@@ -118,13 +118,12 @@
size_t field = HeapLocation::kInvalidFieldOffset;
size_t vec = HeapLocation::kScalar;
size_t class_def = HeapLocation::kDeclaringClassDefIndexForArrays;
- const bool is_vec_op = false;
size_t loc1 = heap_location_collector.FindHeapLocationIndex(
- ref, type, field, c1, vec, class_def, is_vec_op);
+ ref, type, field, c1, vec, class_def);
size_t loc2 = heap_location_collector.FindHeapLocationIndex(
- ref, type, field, c2, vec, class_def, is_vec_op);
+ ref, type, field, c2, vec, class_def);
size_t loc3 = heap_location_collector.FindHeapLocationIndex(
- ref, type, field, index, vec, class_def, is_vec_op);
+ ref, type, field, index, vec, class_def);
// must find this reference info for array in HeapLocationCollector.
ASSERT_TRUE(ref != nullptr);
// must find these heap locations;
diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc
index cd44082..b385e03 100644
--- a/compiler/optimizing/load_store_elimination.cc
+++ b/compiler/optimizing/load_store_elimination.cc
@@ -2113,15 +2113,9 @@
HInstruction* replacement = GetDefaultValue(type);
for (uint32_t phi_placeholder_index : visited.Indexes()) {
DCHECK(phi_placeholder_replacements_[phi_placeholder_index].IsInvalid());
- PhiPlaceholder curr = GetPhiPlaceholderAt(phi_placeholder_index);
- HeapLocation* hl = heap_location_collector_.GetHeapLocation(curr.GetHeapLocation());
- // We use both vector and non vector operations to analyze the information. However, we replace
- // only non vector operations in this code path.
- if (!hl->IsVecOp()) {
- phi_placeholder_replacements_[phi_placeholder_index] = Value::ForInstruction(replacement);
- phi_placeholders_to_materialize->ClearBit(phi_placeholder_index);
- }
+ phi_placeholder_replacements_[phi_placeholder_index] = Value::ForInstruction(replacement);
}
+ phi_placeholders_to_materialize->Subtract(&visited);
return true;
}
@@ -2176,15 +2170,9 @@
DCHECK(replacement != nullptr);
for (uint32_t phi_placeholder_index : visited.Indexes()) {
DCHECK(phi_placeholder_replacements_[phi_placeholder_index].IsInvalid());
- PhiPlaceholder curr = GetPhiPlaceholderAt(phi_placeholder_index);
- HeapLocation* hl = heap_location_collector_.GetHeapLocation(curr.GetHeapLocation());
- // We use both vector and non vector operations to analyze the information. However, we replace
- // only vector operations in this code path.
- if (hl->IsVecOp()) {
- phi_placeholder_replacements_[phi_placeholder_index] = Value::ForInstruction(replacement);
- phi_placeholders_to_materialize->ClearBit(phi_placeholder_index);
- }
+ phi_placeholder_replacements_[phi_placeholder_index] = Value::ForInstruction(replacement);
}
+ phi_placeholders_to_materialize->Subtract(&visited);
return true;
}
diff --git a/test/2256-checker-vector-replacement/expected-stderr.txt b/test/2256-checker-vector-replacement/expected-stderr.txt
deleted file mode 100644
index e69de29..0000000
--- a/test/2256-checker-vector-replacement/expected-stderr.txt
+++ /dev/null
diff --git a/test/2256-checker-vector-replacement/expected-stdout.txt b/test/2256-checker-vector-replacement/expected-stdout.txt
deleted file mode 100644
index e69de29..0000000
--- a/test/2256-checker-vector-replacement/expected-stdout.txt
+++ /dev/null
diff --git a/test/2256-checker-vector-replacement/info.txt b/test/2256-checker-vector-replacement/info.txt
deleted file mode 100644
index 374d7eb..0000000
--- a/test/2256-checker-vector-replacement/info.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-Regression test to check we can perform LSE even when vector and
-non-vector operations reference the same heap location.
diff --git a/test/2256-checker-vector-replacement/src/Main.java b/test/2256-checker-vector-replacement/src/Main.java
deleted file mode 100644
index 43ec0d6..0000000
--- a/test/2256-checker-vector-replacement/src/Main.java
+++ /dev/null
@@ -1,69 +0,0 @@
-/*
- * Copyright (C) 2023 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-public class Main {
- public static void main(String[] args) {
- $noinline$testVectorAndNonVector();
- }
-
- // Before loop optimization we only had an array get. After it, we optimized to also have
- // VecLoad operations.
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() loop_optimization (before)
- /// CHECK: ArrayGet
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() loop_optimization (after)
- /// CHECK: ArrayGet
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() loop_optimization (before)
- /// CHECK-NOT: VecLoad
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() loop_optimization (after)
- /// CHECK: VecLoad
-
- // In LoadStoreElimination both ArrayGet and VecLoad have the same heap location. We will try to
- // replace the ArrayGet with the constant 0. The crash happens when we want to do the same with
- // the vector operation, changing the vector operation to a scalar.
-
- // We can eliminate the ArraySet and ArrayGet, but not the VectorOperations.
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() load_store_elimination (before)
- /// CHECK: ArraySet
- /// CHECK: VecLoad
- /// CHECK: ArrayGet
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() load_store_elimination (after)
- /// CHECK-NOT: ArraySet
-
- /// CHECK-START: void Main.$noinline$testVectorAndNonVector() load_store_elimination (after)
- /// CHECK-NOT: ArrayGet
-
- private static void $noinline$testVectorAndNonVector() {
- int[] result = new int[10];
- int[] source = new int[50];
-
- for (int i = 0; i < result.length; ++i) {
- int value = 0;
- // Always true but needed to repro a crash since we need Phis.
- if (i + 20 < source.length) {
- for (int j = 0; j < 20; j++) {
- value += Math.abs(source[i + j]);
- }
- }
- result[i] = value;
- }
- }
-}