Skip to content

Commit

Permalink
[dart2js] Fix SSA value range analysis bugs
Browse files Browse the repository at this point in the history 
Treat the loop update marker more like an ordinary symbolic value. The old symbolic marker attempted to do widening 'on the fly', which could lead to incorrect results when the update could reset the value to a constant lower than the starting value. The new version moves the widening to a single place, at the loop update.

Fix #53355 by caching intermediate results so that long chains of diamond control flow are not explored exponentially.

There are very few changes in apps. There is one change in a Flutter app that is like the changed codegen/value_range_test where the bounds check can be eliminated because the loop index may be decremented, but not more than the increment, so is still weakly monotonic. There is one change in a large ACX app where a lower bounds check is no longer removed but I *think* it was previously removed incorrectly, though it is hard to tell since it is in a huge function.

Issue: #48465
Issue: #53078
Issue: #53355
Change-Id: Ib125cd6bb30cef52f8dfcd53eaa13e439f26316c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/322594
Reviewed-by: Sigmund Cherem <[email protected]>
Commit-Queue: Stephen Adams <[email protected]>
  • Loading branch information
@rakudrama
rakudrama authored and Commit Queue committed Aug 29, 2023
1 parent ac59672 commit 404edd9
Show file tree
Hide file tree
Showing 5 changed files with 261 additions and 70 deletions.
229 changes: 160 additions & 69 deletions pkg/compiler/lib/src/ssa/value_range_analyzer.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:collection' show UnmodifiableMapView;

import '../constants/constant_system.dart' as constant_system;
import '../constants/values.dart';
import '../js_model/js_world.dart' show JClosedWorld;
import 'nodes.dart';
import 'optimize.dart' show OptimizationPhase, SsaOptimizerTask;

bool _DEBUG = false;

class ValueRangeInfo {
late final IntValue intZero;
late final IntValue intOne;
Expand Down Expand Up @@ -57,8 +61,8 @@ class ValueRangeInfo {
return Range.normalize(low, up, this);
}

Range newMarkerRange() {
return Range(MarkerValue(false, this), MarkerValue(true, this), this);
Value newMarkerValue({required bool isLower, required bool isPositive}) {
return MarkerValue(isLower, isPositive, this);
}
}

Expand Down Expand Up @@ -94,38 +98,15 @@ abstract class Value {
return info.unknownValue;
}

Value replaceMarkers(Value lowerBound, Value upperBound) {
return this;
}

bool get isNegative => false;
bool get isPositive => false;
bool get isZero => false;
}

/// The [MarkerValue] class is used to recognize ranges of loop
/// updates.
class MarkerValue extends Value {
/// If [positive] is true (respectively false), the marker goes
/// to [MaxIntValue] (respectively [MinIntValue]) when being added
/// to a positive (respectively negative) value.
final bool positive;

const MarkerValue(this.positive, info) : super(info);

@override
Value operator +(Value other) {
if (other.isPositive && positive) return info.maxIntValue;
if (other.isNegative && !positive) return info.minIntValue;
if (other is IntValue) return this;
return info.unknownValue;
}

@override
Value operator -(Value other) {
if (other.isPositive && !positive) return info.minIntValue;
if (other.isNegative && positive) return info.maxIntValue;
if (other is IntValue) return this;
return info.unknownValue;
}
}

/// An [IntValue] contains a constant integer value.
class IntValue extends Value {
final BigInt value;
Expand Down Expand Up @@ -210,7 +191,7 @@ class IntValue extends Value {
int get hashCode => throw UnsupportedError('IntValue.hashCode');

@override
String toString() => 'IntValue $value';
String toString() => 'Int($value)';
@override
bool get isNegative => value < BigInt.zero;
@override
Expand Down Expand Up @@ -285,19 +266,14 @@ class UnknownValue extends Value {
String toString() => 'Unknown';
}

/// A symbolic value representing an [HInstruction].
class InstructionValue extends Value {
final HInstruction instruction;
InstructionValue(this.instruction, info) : super(info);
abstract class VariableValue extends Value {
VariableValue(super.info);

@override
bool operator ==(other) {
if (other is! InstructionValue) return false;
return this.instruction == other.instruction;
}
bool operator ==(other) => throw UnsupportedError('VariableValue.==');

@override
int get hashCode => throw UnsupportedError('InstructionValue.hashCode');
int get hashCode => throw UnsupportedError('VariableValue.hashCode');

@override
Value operator +(Value other) {
Expand All @@ -308,7 +284,7 @@ class InstructionValue extends Value {
}
return info.newAddValue(this, other);
}
if (other is InstructionValue) {
if (other is VariableValue) {
return info.newAddValue(this, other);
}
return other + this;
Expand All @@ -324,7 +300,7 @@ class InstructionValue extends Value {
}
return info.newSubtractValue(this, other);
}
if (other is InstructionValue) {
if (other is VariableValue) {
return info.newSubtractValue(this, other);
}
return -other + this;
Expand All @@ -341,7 +317,51 @@ class InstructionValue extends Value {
bool get isPositive => false;

@override
String toString() => 'Instruction: $instruction';
String toString() => throw UnsupportedError('VariableValue.toString');
}

/// The [MarkerValue] class is a symbolc variable used to recognize ranges of
/// loop updates.
class MarkerValue extends VariableValue {
/// There are two marker values in the marker range - a lower value and an
/// upper value. [isLower] is true for the lower marker value.
final bool isLower;
@override
final bool isPositive;

MarkerValue(this.isLower, this.isPositive, super.info);

@override
bool operator ==(other) {
return other is MarkerValue && isLower == other.isLower;
}

@override
Value replaceMarkers(Value lowerBound, Value upperBound) {
return isLower ? lowerBound : upperBound;
}

@override
String toString() =>
'Marker(${isLower ? "lower" : "upper"}${isPositive ? ",>=0" : ""})';
}

/// A symbolic value representing an [HInstruction].
class InstructionValue extends VariableValue {
final HInstruction instruction;
InstructionValue(this.instruction, super.info);

@override
bool operator ==(other) {
if (other is! InstructionValue) return false;
return this.instruction == other.instruction;
}

@override
int get hashCode => throw UnsupportedError('InstructionValue.hashCode');

@override
String toString() => 'Instruction($instruction)';
}

/// Special value for instructions whose type is a positive integer.
Expand Down Expand Up @@ -407,6 +427,14 @@ class AddValue extends BinaryOperationValue {
return info.unknownValue;
}

@override
Value replaceMarkers(Value lowerBound, Value upperBound) {
final newLeft = left.replaceMarkers(lowerBound, upperBound);
final newRight = right.replaceMarkers(lowerBound, upperBound);
if (left == newLeft && right == newRight) return this;
return newLeft + newRight;
}

@override
bool get isNegative => left.isNegative && right.isNegative;
@override
Expand Down Expand Up @@ -462,6 +490,14 @@ class SubtractValue extends BinaryOperationValue {
return info.unknownValue;
}

@override
Value replaceMarkers(Value lowerBound, Value upperBound) {
final newLeft = left.replaceMarkers(lowerBound, upperBound);
final newRight = right.replaceMarkers(lowerBound, upperBound);
if (left == newLeft && right == newRight) return this;
return newLeft - newRight;
}

@override
bool get isNegative => left.isNegative && right.isPositive;
@override
Expand Down Expand Up @@ -522,6 +558,13 @@ class NegateValue extends Value {
@override
Value operator -() => value;

@override
Value replaceMarkers(Value lowerBound, Value upperBound) {
final newValue = value.replaceMarkers(lowerBound, upperBound);
if (value == newValue) return this;
return -newValue;
}

@override
bool get isNegative => value.isPositive;
@override
Expand Down Expand Up @@ -622,6 +665,13 @@ class Range {
}
}

Range replaceMarkers(Value lowerBound, Value upperBound) {
final newLower = lower.replaceMarkers(lowerBound, upperBound);
final newUpper = upper.replaceMarkers(lowerBound, upperBound);
if (lower == newLower && upper == newUpper) return this;
return info.newNormalizedRange(newLower, newUpper);
}

@override
bool operator ==(other) {
if (other is! Range) return false;
Expand Down Expand Up @@ -664,7 +714,7 @@ typedef BinaryRangeOperation = Range Function(Range, Range);
class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
implements OptimizationPhase {
@override
String get name => 'SSA value range builder';
String get name => 'SsaValueRangeAnalyzer';

/// List of [HRangeConversion] instructions created by the phase. We
/// save them here in order to remove them once the phase is done.
Expand All @@ -675,17 +725,19 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
final Map<HInstruction, Range> ranges = {};

final JClosedWorld closedWorld;
final ValueRangeInfo info;
final ValueRangeInfo info = ValueRangeInfo();
final SsaOptimizerTask optimizer;

late HGraph graph;

SsaValueRangeAnalyzer(JClosedWorld closedWorld, this.optimizer)
: info = ValueRangeInfo(),
this.closedWorld = closedWorld;
SsaValueRangeAnalyzer(this.closedWorld, this.optimizer);

@override
void visitGraph(HGraph graph) {
// Example debugging code:
//
// _DEBUG = graph.element.toString().contains('(main)');

this.graph = graph;
visitDominatorTree(graph);
// We remove the range conversions after visiting the graph so
Expand Down Expand Up @@ -741,8 +793,9 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>

@override
Range visitPhi(HPhi phi) {
if (phi.isInteger(closedWorld.abstractValueDomain).isPotentiallyFalse)
if (phi.isInteger(closedWorld.abstractValueDomain).isPotentiallyFalse) {
return info.newUnboundRange();
}
// Some phases may replace instructions that change the inputs of
// this phi. Only the [SsaTypesPropagation] phase will update the
// phi type. Play it safe by assuming the [SsaTypesPropagation]
Expand All @@ -752,7 +805,9 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
return info.newUnboundRange();
}
if (phi.block!.isLoopHeader()) {
final range = LoopUpdateRecognizer(closedWorld, ranges, info).run(phi);
final range =
LoopUpdateRecognizer(closedWorld, UnmodifiableMapView(ranges), info)
.run(phi);
if (range == null) return info.newUnboundRange();
return range;
}
Expand Down Expand Up @@ -1200,30 +1255,65 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
/// Tries to find a range for the update instruction of a loop phi.
class LoopUpdateRecognizer extends HBaseVisitor<Range?> {
final JClosedWorld closedWorld;
final Map<HInstruction, Range> ranges;
// Ranges from outside the loop, which never contain a marker value.
final UnmodifiableMapView<HInstruction, Range> ranges;
final ValueRangeInfo info;

// Ranges inside the loop which may contain marker values specific to the loop
// phi.
final Map<HInstruction, Range?> temporaryRanges = {};

LoopUpdateRecognizer(this.closedWorld, this.ranges, this.info);

Range? run(HPhi loopPhi) {
// Create a marker range for the loop phi, so that if the update
// uses the loop phi, it has a range to use.
ranges[loopPhi] = info.newMarkerRange();
// Create a marker range for the loop phi. This is the symbolic initial
// value of the loop variable for one iteration.
bool isPositive = loopPhi
.isPositiveInteger(closedWorld.abstractValueDomain)
.isDefinitelyTrue;
final lowerMarker =
info.newMarkerValue(isLower: true, isPositive: isPositive);
final upperMarker =
info.newMarkerValue(isLower: false, isPositive: isPositive);
final markerRange = info.newNormalizedRange(lowerMarker, upperMarker);

// Compute the update range as a function of the initial marker range.
temporaryRanges[loopPhi] = markerRange;
final updateRange = visit(loopPhi.inputs[1]);
ranges.remove(loopPhi);
if (updateRange == null) return null;

// Use 'union' to compare the marker with the loop update to find out if the
// lower or upper value did not change.
final deltaRange = markerRange.union(updateRange);

// If the lower (respectively upper) value is the marker, we know the loop
// does not change it, so we can use the [startRange]'s lower (upper) value.
// Otherwise the lower (upper) value changes and needs to be widened to the
// minimum (maximum) value.
final startRange = ranges[loopPhi.inputs[0]]!;
// If the lower (respectively upper) value is the marker, we know
// the loop does not change it, so we can just use the
// [startRange]'s lower (upper) value. Otherwise the lower (upper) value
// is the minimum of the [startRange]'s lower (upper) and the
// [updateRange]'s lower (upper).
Value low = updateRange.lower is MarkerValue
? startRange.lower
: updateRange.lower.min(startRange.lower);
Value up = updateRange.upper is MarkerValue
? startRange.upper
: updateRange.upper.max(startRange.upper);
return info.newNormalizedRange(low, up);

Value lowerLimit = isPositive ? info.intZero : info.minIntValue;
Value upperLimit = info.maxIntValue;
Value lowerBound =
deltaRange.lower == lowerMarker ? startRange.lower : lowerLimit;
Value upperBound =
deltaRange.upper == upperMarker ? startRange.upper : upperLimit;

// Widen the update range and union with the start range.
final widened = updateRange.replaceMarkers(lowerBound, upperBound);
final result = startRange.union(widened);

if (_DEBUG) {
print('------- ${loopPhi.sourceElement}'
'\n marker $markerRange'
'\n update $updateRange'
'\n delta $deltaRange'
'\n start $startRange'
'\n widened $widened'
'\n result= $result');
}

return result;
}

Range? visit(HInstruction instruction) {
Expand All @@ -1232,13 +1322,14 @@ class LoopUpdateRecognizer extends HBaseVisitor<Range?> {
.isPotentiallyFalse) {
return null;
}
return ranges[instruction] ?? instruction.accept(this);
Range? result = ranges[instruction];
if (result != null) return result;
return temporaryRanges[instruction] ??= instruction.accept(this);
}

@override
Range? visitPhi(HPhi phi) {
// If the update of a loop phi involves another loop phi, we give
// up.
// If the update of a loop phi involves another loop phi, we give up.
if (phi.block!.isLoopHeader()) return null;
Range? phiRange;
for (HInstruction input in phi.inputs) {
Expand Down
Loading

0 comments on commit 404edd9

Please sign in to comment.