LSE fix to allow different operation kinds in the same heap location
We had a crash in which a vector operation was replaced by a scalar
value. This CL fixes that and allows vector and non-vector operations
to reference the same heap location.
Note that we can still use the knowledge of these HeapLocations to
propagate values, but we do not replace vector loads with scalar values
or viceversa.
Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Test: dex2oat compiling the app in the bug
Bug: 243136064
Fixes: 243136064
Change-Id: I2cf7d647225e32c111d57140bbca7042a7424667
diff --git a/compiler/optimizing/load_store_analysis.h b/compiler/optimizing/load_store_analysis.h
index c0799db..c46a5b9 100644
--- a/compiler/optimizing/load_store_analysis.h
+++ b/compiler/optimizing/load_store_analysis.h
@@ -171,14 +171,16 @@
size_t offset,
HInstruction* index,
size_t vector_length,
- int16_t declaring_class_def_index)
+ int16_t declaring_class_def_index,
+ bool is_vec_op)
: 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) {
+ has_aliased_locations_(false),
+ is_vec_op_(is_vec_op) {
DCHECK(ref_info != nullptr);
DCHECK((offset == kInvalidFieldOffset && index != nullptr) ||
(offset != kInvalidFieldOffset && index == nullptr));
@@ -189,6 +191,7 @@
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.
@@ -227,11 +230,12 @@
// 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);
};
@@ -318,7 +322,8 @@
field->GetFieldOffset().SizeValue(),
nullptr,
HeapLocation::kScalar,
- field->GetDeclaringClassDefIndex());
+ field->GetDeclaringClassDefIndex(),
+ /*is_vec_op=*/false);
}
size_t GetArrayHeapLocation(HInstruction* instruction) const {
@@ -327,10 +332,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 (instruction->IsVecStore() ||
- instruction->IsVecLoad()) {
+ } else if (is_vec_op) {
HVecOperation* vec_op = instruction->AsVecOperation();
type = vec_op->GetPackedType();
vector_length = vec_op->GetVectorLength();
@@ -342,7 +347,8 @@
HeapLocation::kInvalidFieldOffset,
index,
vector_length,
- HeapLocation::kDeclaringClassDefIndexForArrays);
+ HeapLocation::kDeclaringClassDefIndexForArrays,
+ is_vec_op);
}
bool HasHeapStores() const {
@@ -364,7 +370,8 @@
size_t offset,
HInstruction* index,
size_t vector_length,
- int16_t declaring_class_def_index) const {
+ int16_t declaring_class_def_index,
+ bool is_vec_op) const {
DataType::Type lookup_type = DataType::ToSigned(type);
for (size_t i = 0; i < heap_locations_.size(); i++) {
HeapLocation* loc = heap_locations_[i];
@@ -373,7 +380,8 @@
loc->GetOffset() == offset &&
loc->GetIndex() == index &&
loc->GetVectorLength() == vector_length &&
- loc->GetDeclaringClassDefIndex() == declaring_class_def_index) {
+ loc->GetDeclaringClassDefIndex() == declaring_class_def_index &&
+ loc->IsVecOp() == is_vec_op) {
return i;
}
}
@@ -518,14 +526,15 @@
size_t offset,
HInstruction* index,
size_t vector_length,
- int16_t declaring_class_def_index) {
+ int16_t declaring_class_def_index,
+ bool is_vec_op) {
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);
+ ref_info, type, offset, index, vector_length, declaring_class_def_index, is_vec_op);
if (heap_location_idx == kHeapLocationNotFound) {
- HeapLocation* heap_loc = new (allocator_)
- HeapLocation(ref_info, type, offset, index, vector_length, declaring_class_def_index);
+ HeapLocation* heap_loc = new (allocator_) HeapLocation(
+ ref_info, type, offset, index, vector_length, declaring_class_def_index, is_vec_op);
heap_locations_.push_back(heap_loc);
}
}
@@ -539,19 +548,22 @@
offset,
nullptr,
HeapLocation::kScalar,
- declaring_class_def_index);
+ declaring_class_def_index,
+ /*is_vec_op=*/false);
}
void VisitArrayAccess(HInstruction* array,
HInstruction* index,
DataType::Type type,
- size_t vector_length) {
+ size_t vector_length,
+ bool is_vec_op) {
MaybeCreateHeapLocation(array,
type,
HeapLocation::kInvalidFieldOffset,
index,
vector_length,
- HeapLocation::kDeclaringClassDefIndexForArrays);
+ HeapLocation::kDeclaringClassDefIndexForArrays,
+ is_vec_op);
}
void VisitPredicatedInstanceFieldGet(HPredicatedInstanceFieldGet* instruction) override {
@@ -585,7 +597,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetType();
- VisitArrayAccess(array, index, type, HeapLocation::kScalar);
+ VisitArrayAccess(array, index, type, HeapLocation::kScalar, /*is_vec_op=*/false);
CreateReferenceInfoForReferenceType(instruction);
}
@@ -593,7 +605,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetComponentType();
- VisitArrayAccess(array, index, type, HeapLocation::kScalar);
+ VisitArrayAccess(array, index, type, HeapLocation::kScalar, /*is_vec_op=*/false);
has_heap_stores_ = true;
}
@@ -601,7 +613,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetPackedType();
- VisitArrayAccess(array, index, type, instruction->GetVectorLength());
+ VisitArrayAccess(array, index, type, instruction->GetVectorLength(), /*is_vec_op=*/true);
CreateReferenceInfoForReferenceType(instruction);
}
@@ -609,7 +621,7 @@
HInstruction* array = instruction->InputAt(0);
HInstruction* index = instruction->InputAt(1);
DataType::Type type = instruction->GetPackedType();
- VisitArrayAccess(array, index, type, instruction->GetVectorLength());
+ VisitArrayAccess(array, index, type, instruction->GetVectorLength(), /*is_vec_op=*/true);
has_heap_stores_ = true;
}
diff --git a/compiler/optimizing/load_store_analysis_test.cc b/compiler/optimizing/load_store_analysis_test.cc
index 7930c0d..b6cf8b4 100644
--- a/compiler/optimizing/load_store_analysis_test.cc
+++ b/compiler/optimizing/load_store_analysis_test.cc
@@ -118,12 +118,13 @@
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);
+ ref, type, field, c1, vec, class_def, is_vec_op);
size_t loc2 = heap_location_collector.FindHeapLocationIndex(
- ref, type, field, c2, vec, class_def);
+ ref, type, field, c2, vec, class_def, is_vec_op);
size_t loc3 = heap_location_collector.FindHeapLocationIndex(
- ref, type, field, index, vec, class_def);
+ ref, type, field, index, vec, class_def, is_vec_op);
// 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 b385e03..cd44082 100644
--- a/compiler/optimizing/load_store_elimination.cc
+++ b/compiler/optimizing/load_store_elimination.cc
@@ -2113,9 +2113,15 @@
HInstruction* replacement = GetDefaultValue(type);
for (uint32_t phi_placeholder_index : visited.Indexes()) {
DCHECK(phi_placeholder_replacements_[phi_placeholder_index].IsInvalid());
- phi_placeholder_replacements_[phi_placeholder_index] = Value::ForInstruction(replacement);
+ 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_placeholders_to_materialize->Subtract(&visited);
return true;
}
@@ -2170,9 +2176,15 @@
DCHECK(replacement != nullptr);
for (uint32_t phi_placeholder_index : visited.Indexes()) {
DCHECK(phi_placeholder_replacements_[phi_placeholder_index].IsInvalid());
- phi_placeholder_replacements_[phi_placeholder_index] = Value::ForInstruction(replacement);
+ 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_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
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/2256-checker-vector-replacement/expected-stderr.txt
diff --git a/test/2256-checker-vector-replacement/expected-stdout.txt b/test/2256-checker-vector-replacement/expected-stdout.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/2256-checker-vector-replacement/expected-stdout.txt
diff --git a/test/2256-checker-vector-replacement/info.txt b/test/2256-checker-vector-replacement/info.txt
new file mode 100644
index 0000000..374d7eb
--- /dev/null
+++ b/test/2256-checker-vector-replacement/info.txt
@@ -0,0 +1,2 @@
+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
new file mode 100644
index 0000000..43ec0d6
--- /dev/null
+++ b/test/2256-checker-vector-replacement/src/Main.java
@@ -0,0 +1,69 @@
+/*
+ * 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;
+ }
+ }
+}