[dart2js] Improve DominatedUses.replaceWith

When the instruction being replaced has many uses, it is beneficial to update them together, rather than one at a time.

Also:

- Make value_range_analyer output intermediate state for `--dump-ssa`.
- Remove bounds checks all together at end of range analysis.
- Don't add RangeConversion for the length when the length is a constant.

Compiling a method with 5000 assignments of the form `a[N] = N;` is reduced 20x, from 123 seconds to 6 seconds.

Bug: #56919
Change-Id: I9ca7b41ca156ee40b96cfed9e18369662053b3b7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/390824
Reviewed-by: Nate Biggs <[email protected]>
Commit-Queue: Stephen Adams <[email protected]>

Author: rakudrama

pkg/compiler/lib/src/ssa/nodes.dart

@@ -1138,6 +1138,14 @@ abstract class HInstruction implements SpannableWithEntity {
   // devirtualized.
   final List<HInstruction> inputs;
 
+  // Instructions that uses this instruction. A user is [usedBy] once per input
+  // that is this instruction.
+  //
+  //     y = [x, x + 1, x];
+  //
+  // The [usedBy] for the instruction `x` has three elements, one for the
+  // addition instruction and two for the list literal instruction, in no
+  // particilar order.
   final List<HInstruction> usedBy = [];
 
   HBasicBlock? block;
@@ -1503,19 +1511,63 @@ class DominatedUses {
   bool get isNotEmpty => !isEmpty;
   int get length => _instructions.length;
 
-  /// Changes all the uses in the set to [newInstruction].
-  void replaceWith(HInstruction newInstruction) {
-    assert(!identical(newInstruction, _source));
+  /// Changes all the uses in the set to [replacement].
+  void replaceWith(HInstruction replacement) {
+    assert(replacement.isInBasicBlock());
+    assert(!identical(replacement, _source));
     if (isEmpty) return;
+
     for (int i = 0; i < _instructions.length; i++) {
       HInstruction user = _instructions[i];
       int index = _indexes[i];
-      HInstruction oldInstruction = user.inputs[index];
       assert(
-          identical(oldInstruction, _source),
+          identical(user.inputs[index], _source),
           'Input ${index} of ${user} changed.'
-          '\n  Found: ${oldInstruction}\n  Expected: ${_source}');
-      user.replaceInput(index, newInstruction);
+          '\n  Found: ${user.inputs[index]}\n  Expected: ${_source}');
+      user.inputs[index] = replacement;
+      replacement.usedBy.add(user);
+    }
+
+    // The following loop is a more efficient implementation of:
+    //
+    //     for (final user in _instructions) {
+    //       _source.usedBy.remove(user);
+    //     }
+    //
+    // `List.remove` searches the list to find the key, and then scans the rest
+    // of the list to move the elements up one position.  Repeating this is
+    // quadratic.
+    //
+    // The code below combines searching for the next element with move-up
+    // scanning for the previous element(s) to remove several elements in one
+    // pass, provided elements of `_instructions` are in the same order as in
+    // `usedBy`. This is usually the case since the DominatedUses set is
+    // constructed from `_source.usedBy`.
+
+    final usedBy = _source.usedBy;
+    int instructionsIndex = 0;
+    while (instructionsIndex < _instructions.length) {
+      HInstruction nextToRemove = _instructions[instructionsIndex];
+      int readIndex = 0, writeIndex = 0;
+      while (readIndex < usedBy.length) {
+        final user = usedBy[readIndex++];
+        if (identical(user, nextToRemove)) {
+          instructionsIndex++;
+          if (instructionsIndex < _instructions.length) {
+            nextToRemove = _instructions[instructionsIndex];
+          } else {
+            // Copy rest of the list elements up as-is.
+            while (readIndex < usedBy.length) {
+              usedBy[writeIndex++] = usedBy[readIndex++];
+            }
+            break;
+          }
+        } else {
+          usedBy[writeIndex++] = user;
+        }
+      }
+      assert(writeIndex < readIndex, 'Should remove at least one per pass');
+      usedBy.length = writeIndex;
     }
   }
 
@@ -1532,11 +1584,13 @@ class DominatedUses {
 
   void _compute(HInstruction source, HInstruction dominator,
       bool excludeDominator, bool excludePhiOutEdges) {
+    assert(dominator is! HPhi);
+
     // Keep track of all instructions that we have to deal with later and count
-    // the number of them that are in the current block.
+    // the number of them that are in the dominator's block.
     Set<HInstruction> users = Setlet();
     Set<HInstruction> seen = Setlet();
-    int usersInCurrentBlock = 0;
+    int usersInDominatorBlock = 0;
 
     HBasicBlock dominatorBlock = dominator.block!;
 
@@ -1547,9 +1601,15 @@ class DominatedUses {
     for (HInstruction current in source.usedBy) {
       if (!seen.add(current)) continue;
       HBasicBlock currentBlock = current.block!;
-      if (dominatorBlock.dominates(currentBlock)) {
+      if (identical(currentBlock, dominatorBlock)) {
+        // Ignore phi nodes of the dominator instruction block, they come before
+        // the dominator instruction.
+        if (current is! HPhi) {
+          users.add(current);
+          usersInDominatorBlock++;
+        }
+      } else if (dominatorBlock.dominates(currentBlock)) {
         users.add(current);
-        if (identical(currentBlock, dominatorBlock)) usersInCurrentBlock++;
       } else if (!excludePhiOutEdges && current is HPhi) {
         // A non-dominated HPhi.
         // See if there a dominated edge into the phi. The input must be
@@ -1565,25 +1625,14 @@ class DominatedUses {
       }
     }
 
-    // Run through all the phis in the same block as [dominator] and remove them
-    // from the users set. These come before [dominator].
-    // TODO(sra): Could we simply not add them in the first place?
-    if (usersInCurrentBlock > 0) {
-      for (var phi = dominatorBlock.phis.first; phi != null; phi = phi.next) {
-        if (users.remove(phi)) {
-          if (--usersInCurrentBlock == 0) break;
-        }
-      }
-    }
-
     // Run through all the instructions before [dominator] and remove them from
     // the users set.
-    if (usersInCurrentBlock > 0) {
+    if (usersInDominatorBlock > 0) {
       for (var current = dominatorBlock.first;
           !identical(current, dominator);
           current = current!.next) {
         if (users.remove(current)) {
-          if (--usersInCurrentBlock == 0) break;
+          if (--usersInDominatorBlock == 0) break;
         }
       }
       if (excludeDominator) {

pkg/compiler/lib/src/ssa/optimize.dart

@@ -132,7 +132,7 @@ class SsaOptimizerTask extends CompilerTask {
         SsaTypeConversionInserter(closedWorld),
         SsaTypePropagator(globalInferenceResults, closedWorld.commonElements,
             closedWorld, log),
-        SsaValueRangeAnalyzer(closedWorld, this),
+        SsaValueRangeAnalyzer(closedWorld, this, codegen.tracer),
         // Previous optimizations may have generated new
         // opportunities for instruction simplification.
         SsaInstructionSimplifier(globalInferenceResults, _options, closedWorld,
@@ -156,7 +156,7 @@ class SsaOptimizerTask extends CompilerTask {
               closedWorld, log),
           SsaGlobalValueNumberer(closedWorld.abstractValueDomain),
           SsaCodeMotion(closedWorld.abstractValueDomain),
-          SsaValueRangeAnalyzer(closedWorld, this),
+          SsaValueRangeAnalyzer(closedWorld, this, codegen.tracer),
           SsaInstructionSimplifier(globalInferenceResults, _options,
               closedWorld, typeRecipeDomain, registry, log, metrics),
           SsaSimplifyInterceptors(closedWorld, member.enclosingClass),

pkg/compiler/lib/src/ssa/tracer.dart

@@ -242,7 +242,8 @@ class HInstructionStringifier implements HVisitor<String> {
   String visitBoundsCheck(HBoundsCheck node) {
     String lengthId = temporaryId(node.length);
     String indexId = temporaryId(node.index);
-    return "BoundsCheck: length = $lengthId, index = $indexId";
+    return 'BoundsCheck: length = $lengthId, index = $indexId'
+        ', ${node.staticChecks.name}';
   }
 
   @override
@@ -713,7 +714,8 @@ class HInstructionStringifier implements HVisitor<String> {
 
   @override
   String visitRangeConversion(HRangeConversion node) {
-    return "RangeConversion: ${node.checkedInput}";
+    var inputs = node.inputs.map(temporaryId).join(', ');
+    return 'RangeConversion: $inputs';
   }
 
   @override

pkg/compiler/lib/src/ssa/value_range_analyzer.dart

@@ -7,6 +7,7 @@ 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 '../tracer.dart';
 import 'nodes.dart';
 import 'optimize.dart' show OptimizationPhase, SsaOptimizerTask;
 
@@ -720,17 +721,21 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
   /// save them here in order to remove them once the phase is done.
   final List<HRangeConversion> conversions = [];
 
+  /// List of [HBoundsCheck] instructions collected during visit.
+  final List<HBoundsCheck> boundsChecks = [];
+
   /// Value ranges for integer instructions. This map gets populated by
   /// the dominator tree visit.
   final Map<HInstruction, Range> ranges = {};
 
   final JClosedWorld closedWorld;
   final ValueRangeInfo info = ValueRangeInfo();
   final SsaOptimizerTask optimizer;
+  final Tracer tracer;
 
   late HGraph graph;
 
-  SsaValueRangeAnalyzer(this.closedWorld, this.optimizer);
+  SsaValueRangeAnalyzer(this.closedWorld, this.optimizer, this.tracer);
 
   @override
   void visitGraph(HGraph graph) {
@@ -740,23 +745,39 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
 
     this.graph = graph;
     visitDominatorTree(graph);
+
+    tracer.traceGraph(name + '.analysis', graph);
+
+    optimizeBoundsChecks();
+
     // We remove the range conversions after visiting the graph so
     // that the graph does not get polluted with these instructions
     // only necessary for this phase.
     removeRangeConversion();
+
     // TODO(herhut): Find a cleaner way to pass around ranges.
     optimizer.ranges = ranges;
   }
 
   @override
   bool validPostcondition(HGraph graph) => true;
 
+  void optimizeBoundsChecks() {
+    for (final check in boundsChecks) {
+      if (check.staticChecks == StaticBoundsChecks.alwaysTrue) {
+        final block = check.block!;
+        block.rewrite(check, check.index);
+        block.remove(check);
+      }
+    }
+  }
+
   void removeRangeConversion() {
-    conversions.forEach((HRangeConversion instruction) {
+    for (final instruction in conversions) {
       final block = instruction.block!;
       block.rewrite(instruction, instruction.inputs[0]);
       block.remove(instruction);
-    });
+    }
   }
 
   @override
@@ -867,6 +888,7 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
 
   @override
   Range visitBoundsCheck(HBoundsCheck check) {
+    boundsChecks.add(check);
     // Save the next instruction, in case the check gets removed.
     final next = check.next;
     Range? indexRange = ranges[check.index];
@@ -899,9 +921,7 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
         (indexRange.upper != lengthRange.lower &&
             indexRange.upper.min(lengthRange.lower) == indexRange.upper);
     if (indexRange.isPositive && belowLength) {
-      final checkBlock = check.block!;
-      checkBlock.rewrite(check, check.index);
-      checkBlock.remove(check);
+      check.staticChecks = StaticBoundsChecks.alwaysTrue;
     } else if (indexRange.isNegative || lengthRange < indexRange) {
       check.staticChecks = StaticBoundsChecks.alwaysFalse;
       // The check is always false, and whatever instruction it
@@ -917,7 +937,7 @@ class SsaValueRangeAnalyzer extends HBaseVisitor<Range>
       // If the test passes, we know the lower bound of the length is
       // greater or equal than the lower bound of the index.
       Value low = lengthRange.lower.max(indexRange.lower);
-      if (low != info.unknownValue) {
+      if (low != info.unknownValue && check.length is! HConstant) {
         HInstruction instruction = createRangeConversion(next!, check.length);
         ranges[instruction] = info.newNormalizedRange(low, lengthRange.upper);
       }