GenericCheckArrayBound elimination does not use range analysis results #37687
Description
This seems a regression from older implementation which used to keep bounds checks represented as CheckArrayBound until much later in pipeline and then converted them into GenericCheckArrayBound.
In the code below I would expect to see a single bounds check, however in AOT version I see two.
double foo(Float64List l) {
return l[1] + l[0];
}In general it seems a bit weird to have two different IsRedundant methods - I think they should be merged into a single method.
I have made prototype of the fix:
diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h
index ded19366d0..4a04b8ca1a 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -7696,6 +7696,8 @@ class CheckBoundBase : public TemplateDefinition<2, NoThrow, Pure> {
// Give a name to the location/input indices.
enum { kLengthPos = 0, kIndexPos = 1 };
+ bool IsRedundantBasedOnRangeInformation(const RangeBoundary& length);
+
private:
DISALLOW_COPY_AND_ASSIGN(CheckBoundBase);
};
diff --git a/runtime/vm/compiler/backend/range_analysis.cc b/runtime/vm/compiler/backend/range_analysis.cc
index f99ae1fa2e..c1794d4a28 100644
--- a/runtime/vm/compiler/backend/range_analysis.cc
+++ b/runtime/vm/compiler/backend/range_analysis.cc
@@ -1317,16 +1317,17 @@ void RangeAnalysis::EliminateRedundantBoundsChecks() {
diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h
index ded19366d0..4a04b8ca1a 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -7696,6 +7696,8 @@ class CheckBoundBase : public TemplateDefinition<2, NoThrow, Pure> {
// Give a name to the location/input indices.
enum { kLengthPos = 0, kIndexPos = 1 };
+ bool IsRedundantBasedOnRangeInformation(const RangeBoundary& length);
+
private:
DISALLOW_COPY_AND_ASSIGN(CheckBoundBase);
};
diff --git a/runtime/vm/compiler/backend/range_analysis.cc b/runtime/vm/compiler/backend/range_analysis.cc
index f99ae1fa2e..c1794d4a28 100644
--- a/runtime/vm/compiler/backend/range_analysis.cc
+++ b/runtime/vm/compiler/backend/range_analysis.cc
@@ -1317,16 +1317,17 @@ void RangeAnalysis::EliminateRedundantBoundsChecks() {
for (intptr_t i = 0; i < bounds_checks_.length(); i++) {
// Is this a non-speculative check bound?
- auto aot_check = bounds_checks_[i]->AsGenericCheckBound();
- if (aot_check != nullptr) {
+ if (auto aot_check = bounds_checks_[i]->AsGenericCheckBound()) {
auto length = aot_check->length()
->definition()
->OriginalDefinitionIgnoreBoxingAndConstraints();
auto array_length = RangeBoundary::FromDefinition(length);
- if (aot_check->IsRedundant(array_length)) {
+ if (aot_check->IsRedundant(array_length) ||
+ aot_check->IsRedundantBasedOnRangeInformation(RangeBoundary::FromDefinition(aot_check->length()->definition()))) {
aot_check->ReplaceUsesWith(aot_check->index()->definition());
aot_check->RemoveFromGraph();
}
+
continue;
}
// Must be a speculative check bound.
@@ -2931,7 +2932,7 @@ void IntConverterInstr::InferRange(RangeAnalysis* analysis, Range* range) {
}
}
-bool CheckArrayBoundInstr::IsRedundant(const RangeBoundary& length) {
+bool CheckBoundBase::IsRedundantBasedOnRangeInformation(const RangeBoundary& length) {
Range* index_range = index()->definition()->range();
// Range of the index is unknown can't decide if the check is redundant.
@@ -2985,6 +2986,10 @@ bool CheckArrayBoundInstr::IsRedundant(const RangeBoundary& length) {
return false;
}
+bool CheckArrayBoundInstr::IsRedundant(const RangeBoundary& length) {
+ return IsRedundantBasedOnRangeInformation(length);
+}
+
// Check if range boundary and invariant limit are the same boundary.
static bool IsSameBound(const RangeBoundary& a, InductionVar* b) {
ASSERT(InductionVar::IsInvariant(b));But I currently have enough of time on my hands to properly land this and add tests. I think the proper fix is just to merge IsRedundant into a single method on the base class.
(Important caveat: for range based bounds check elimination it is important to not unwrap length before creating boundary - because it can be constrained).