[dart2js] Better dead phi removal

Remove dead phis including back-edges and cycles.

Add a postcondition validation for optimization phases.

Add post-phase validation test for dead-phi removal and dead-code elimination.

Bug: https://github.com/dart-lang/sdk/issues/48397

Change-Id: I178cda52dd165e825b9e2d9ae642c22162cee2e3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/232782
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Stephen Adams <sra@google.com>

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

@@ -48,6 +48,9 @@ class SsaFinalizeInterceptors extends HBaseVisitor
     visitDominatorTree(graph);
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   @override
   visitBasicBlock(HBasicBlock node) {
     HInstruction instruction = node.first;

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

@@ -46,6 +46,9 @@ class SsaSimplifyInterceptors extends HBaseVisitor
     visitDominatorTree(graph);
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   @override
   void visitBasicBlock(HBasicBlock node) {
     currentBlock = node;

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

@@ -39,12 +39,14 @@ import 'logging.dart';
 import 'nodes.dart';
 import 'types.dart';
 import 'types_propagation.dart';
+import 'validate.dart' show NoUnusedPhiValidator;
 import 'value_range_analyzer.dart';
 import 'value_set.dart';
 
 abstract class OptimizationPhase {
   String get name;
   void visitGraph(HGraph graph);
+  bool validPostcondition(HGraph graph);
 }
 
 class SsaOptimizerTask extends CompilerTask {
@@ -70,6 +72,8 @@ class SsaOptimizerTask extends CompilerTask {
       measureSubtask(phase.name, () => phase.visitGraph(graph));
       codegen.tracer.traceGraph(phase.name, graph);
       assert(graph.isValid(), 'Graph not valid after ${phase.name}');
+      assert(phase.validPostcondition(graph),
+          'Graph does not satify phase postcondition after ${phase.name}');
     }
 
     SsaCodeMotion codeMotion;
@@ -243,6 +247,9 @@ class SsaInstructionSimplifier extends HBaseVisitor
     visitDominatorTree(visitee);
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   @override
   visitBasicBlock(HBasicBlock block) {
     simplifyPhis(block);
@@ -2517,6 +2524,11 @@ class SsaDeadCodeEliminator extends HGraphVisitor implements OptimizationPhase {
     cleanPhis();
   }
 
+  @override
+  bool validPostcondition(HGraph graph) {
+    return NoUnusedPhiValidator.containsNoUnusedPhis(graph);
+  }
+
   @override
   void visitBasicBlock(HBasicBlock block) {
     bool isDeadBlock = analyzer.isDeadBlock(block);
@@ -2827,27 +2839,33 @@ class SsaDeadPhiEliminator implements OptimizationPhase {
       }
     }
 
-    // Remove phis that are not live.
-    // Traverse in reverse order to remove phis with no uses before the
-    // phis that they might use.
-    // NOTICE: Doesn't handle circular references, but we don't currently
-    // create any.
-    List<HBasicBlock> blocks = graph.blocks;
-    for (int i = blocks.length - 1; i >= 0; i--) {
-      HBasicBlock block = blocks[i];
-      HPhi current = block.phis.first;
-      HPhi next = null;
-      while (current != null) {
-        next = current.next;
-        if (!livePhis.contains(current)
-            // TODO(ahe): Not sure the following is correct.
-            &&
-            current.usedBy.isEmpty) {
-          block.removePhi(current);
-        }
-        current = next;
-      }
+    // Collect dead phis.
+    List<HPhi> deadPhis = [];
+    for (final block in graph.blocks) {
+      block.forEachPhi((phi) {
+        if (!livePhis.contains(phi)) deadPhis.add(phi);
+      });
     }
+
+    // Two-phase removal, phase 1: unlink all the input nodes.
+    for (final phi in deadPhis) {
+      for (final input in phi.inputs) {
+        input.removeUser(phi);
+      }
+      phi.inputs.clear();
+    }
+
+    // Two-phase removal, phase 2: remove the now-disconnected phis.
+    for (final phi in deadPhis) {
+      assert(phi.inputs.isEmpty);
+      assert(phi.usedBy.isEmpty);
+      phi.block.removePhi(phi);
+    }
+  }
+
+  @override
+  bool validPostcondition(HGraph graph) {
+    return NoUnusedPhiValidator.containsNoUnusedPhis(graph);
   }
 }
 
@@ -2903,6 +2921,9 @@ class SsaRedundantPhiEliminator implements OptimizationPhase {
       }
     }
   }
+
+  @override
+  bool validPostcondition(HGraph graph) => true;
 }
 
 class GvnWorkItem {
@@ -2933,6 +2954,9 @@ class SsaGlobalValueNumberer implements OptimizationPhase {
     } while (!workQueue.isEmpty);
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   void moveLoopInvariantCode(HGraph graph) {
     for (int i = graph.blocks.length - 1; i >= 0; i--) {
       HBasicBlock block = graph.blocks[i];
@@ -3157,6 +3181,9 @@ class SsaCodeMotion extends HBaseVisitor implements OptimizationPhase {
     visitPostDominatorTree(graph);
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   @override
   void visitBasicBlock(HBasicBlock block) {
     List<HBasicBlock> successors = block.successors;
@@ -3261,6 +3288,9 @@ class SsaTypeConversionInserter extends HBaseVisitor
     visitDominatorTree(graph);
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   // Update users of [input] that are dominated by [:dominator.first:]
   // to use [TypeKnown] of [input] instead. As the type information depends
   // on the control flow, we mark the inserted [HTypeKnown] nodes as
@@ -3443,6 +3473,9 @@ class SsaLoadElimination extends HBaseVisitor implements OptimizationPhase {
     }
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   @override
   void visitBasicBlock(HBasicBlock block) {
     final predecessors = block.predecessors;

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

@@ -70,6 +70,9 @@ class SsaTypePropagator extends HBaseVisitor implements OptimizationPhase {
     processWorklist();
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   @override
   visitBasicBlock(HBasicBlock block) {
     if (block.isLoopHeader()) {

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

@@ -220,3 +220,28 @@ class HValidator extends HInstructionVisitor {
     }
   }
 }
+
+/// Validate that the graph contains no unused phi nodes.
+///
+///     assert(NoUnusedPhiValidator.containsNoUnusedPhis(graph));
+class NoUnusedPhiValidator extends HGraphVisitor {
+  bool isValid = true;
+
+  static bool containsNoUnusedPhis(HGraph graph) {
+    final validator = NoUnusedPhiValidator();
+    validator.visitDominatorTree(graph);
+    return validator.isValid;
+  }
+
+  @override
+  void visitBasicBlock(HBasicBlock block) {
+    block.forEachPhi(visitPhi);
+  }
+
+  void visitPhi(HPhi phi) {
+    if (phi.usedBy.isEmpty) {
+      print('Unused $phi in B${phi.block.id}');
+      isValid = false;
+    }
+  }
+}

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

@@ -665,6 +665,9 @@ class SsaValueRangeAnalyzer extends HBaseVisitor implements OptimizationPhase {
     optimizer.ranges = ranges;
   }
 
+  @override
+  bool validPostcondition(HGraph graph) => true;
+
   void removeRangeConversion() {
     conversions.forEach((HRangeConversion instruction) {
       instruction.block.rewrite(instruction, instruction.inputs[0]);