Reapply "[dart2js] Replace phi with controlling condition"

This reverts commit aec3ec0244.

- Corrected 'controlling condition' detection.
- Added test that is incorrectly compiled to infinite loop with incorrect controlling condition detection.

See https://github.com/dart-lang/sdk/issues/54115#issuecomment-1944285230 for an image of part of the CFG for `doWhileLoop` where the condition in B4 was previously mis-identified as controlling `phi(true,false)` at B12.

Issue: #54115
Change-Id: I0d2c2ff83b202071f6d7050d34de8ff25d05cb22
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352443
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Stephen Adams <sra@google.com>

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

@@ -333,13 +333,69 @@ class SsaInstructionSimplifier extends HBaseVisitor<HInstruction>
   }
 
   // Simplify some CFG diamonds to equivalent expressions.
-  simplifyPhis(HBasicBlock block) {
-    // Is [block] the join point for a simple diamond that generates a single
-    // phi node?
-    if (block.phis.isEmpty) return;
-    HPhi phi = block.phis.first as HPhi;
-    if (phi.next != null) return;
+  void simplifyPhis(HBasicBlock block) {
     if (block.predecessors.length != 2) return;
+
+    // Do 'statement' simplifications first, as they might reduce the number of
+    // phis to one, enabling an 'expression' simplification.
+    HInstruction? phi = block.phis.first;
+    while (phi != null) {
+      final next = phi.next;
+      simplifyStatementPhi(block, phi as HPhi);
+      phi = next;
+    }
+
+    phi = block.phis.first;
+    if (phi != null && phi.next == null) {
+      simplifyExpressionPhi(block, phi as HPhi);
+    }
+  }
+
+  /// Simplify a single phi when there are possibly other phis (i.e. the result
+  /// might not be an expression).
+  void simplifyStatementPhi(HBasicBlock block, HPhi phi) {
+    HBasicBlock dominator = block.dominator!;
+
+    // Extract the controlling condition.
+    final controlFlow = dominator.last;
+    if (controlFlow is! HIf) return;
+    HInstruction condition = controlFlow.inputs.single;
+
+    if (condition.isBoolean(_abstractValueDomain).isPotentiallyFalse) return;
+
+    // For the condition to be 'controlling', there must be no way to reach the
+    // 'else' join from the 'then' branch and vice versa.
+    if (!dominator.successors[0].dominates(block.predecessors[0])) return;
+    if (!dominator.successors[1].dominates(block.predecessors[1])) return;
+
+    //  condition ? true : false  -->  condition
+    //  condition ? condition : false  -->  condition
+    //  condition ? true : condition  -->  condition
+    final left = phi.inputs[0];
+    final right = phi.inputs[1];
+    if ((_isBoolConstant(left, true) || left == condition) &&
+        (_isBoolConstant(right, false) || right == condition)) {
+      block.rewrite(phi, condition);
+      block.removePhi(phi);
+      condition.sourceElement ??= phi.sourceElement;
+      return;
+    }
+
+    //  condition ? false : true  -->  !condition
+    if (_isBoolConstant(left, false) && _isBoolConstant(right, true)) {
+      HInstruction replacement = HNot(condition, _abstractValueDomain.boolType)
+        ..sourceElement = phi.sourceElement
+        ..sourceInformation = phi.sourceInformation;
+      block.addAtEntry(replacement);
+      block.rewrite(phi, replacement);
+      block.removePhi(phi);
+      return;
+    }
+  }
+
+  /// Simplify some CFG diamonds to equivalent expressions.
+  void simplifyExpressionPhi(HBasicBlock block, HPhi phi) {
+    // Is [block] the join point for a simple diamond?
     assert(phi.inputs.length == 2);
     HBasicBlock b1 = block.predecessors[0];
     HBasicBlock b2 = block.predecessors[1];
@@ -350,6 +406,7 @@ class SsaInstructionSimplifier extends HBaseVisitor<HInstruction>
     final controlFlow = dominator.last;
     if (controlFlow is! HIf) return;
     HInstruction test = controlFlow.inputs.single;
+
     if (test.usedBy.length > 1) return;
 
     bool negated = false;
@@ -396,7 +453,7 @@ class SsaInstructionSimplifier extends HBaseVisitor<HInstruction>
       block.removePhi(phi);
       return;
     }
-    // If 'x'is nullable boolean,
+    // If 'x' is nullable boolean,
     //
     //     x == null ? true : x  --->  !(x == false)
     //

pkg/compiler/test/codegen/data/logical_operators.dart

@@ -59,6 +59,20 @@ int noopOr(int a) {
   return (a == 1 || a == 3 || a == 5 || a == 7 || a == 9) ? 100 : 100;
 }
 
+/*member: constantFoldedControlFlow3:function(a) {
+  return a;
+}*/
+bool constantFoldedControlFlow3(bool a) {
+  return a && 1 == 1 && 2 == 2;
+}
+
+/*member: constantFoldedControlFlow4:function(a) {
+  return a;
+}*/
+bool constantFoldedControlFlow4(bool a) {
+  return 1 == 1 && a && 2 == 2;
+}
+
 // Problem cases.
 //
 // Move the following cases above this comment when the code quality improves.
@@ -130,29 +144,15 @@ bool constantFoldedControlFlow1(bool a, bool b) {
 }
 
 /*member: constantFoldedControlFlow2:function(a, b) {
-  return a && b && true;
-}*/
-bool constantFoldedControlFlow2(bool a, bool b) {
-  return 1 == 1 && a && 2 == 2 && b && 3 == 3;
-}
-
-/*member: constantFoldedControlFlow3:function(a) {
   var t1;
   if (a)
-    t1 = true;
+    t1 = b;
   else
     t1 = false;
   return t1;
 }*/
-bool constantFoldedControlFlow3(bool a) {
-  return a && 1 == 1 && 2 == 2;
-}
-
-/*member: constantFoldedControlFlow4:function(a) {
-  return a && true;
-}*/
-bool constantFoldedControlFlow4(bool a) {
-  return 1 == 1 && a && 2 == 2;
+bool constantFoldedControlFlow2(bool a, bool b) {
+  return 1 == 1 && a && 2 == 2 && b && 3 == 3;
 }
 
 @pragma('dart2js:disable-inlining')

pkg/compiler/test/codegen/data/phi_to_condition_test.dart

@@ -0,0 +1,29 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// 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.
+
+@pragma('dart2js:never-inline')
+/*member: foo1:function(a, b) {
+  var changed = a !== b;
+  if (changed)
+    A.log("changed");
+  return changed;
+}*/
+foo1(int a, int b) {
+  bool changed = false;
+  if (a != b) {
+    changed = true;
+    log('changed');
+  }
+  return changed;
+}
+
+@pragma('dart2js:never-inline')
+/*member: log:ignore*/
+void log(String s) {}
+
+/*member: main:ignore*/
+main() {
+  foo1(1, 2);
+  foo1(2, 1);
+}

tests/web/regress/issue/54115_loop_test.dart

@@ -0,0 +1,29 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// 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.
+
+var canSelect = false;
+// Use late binding so the compiler doesn't inline and optimize out the bug.
+late bool Function() select = () => true;
+
+void doWhileLoop() {
+  bool shouldRepeat;
+  do {
+    shouldRepeat = false;
+    if (canSelect) {
+      final isApproved = select();
+      if (!isApproved) {
+        shouldRepeat = true;
+        continue;
+      }
+    }
+  } while (shouldRepeat);
+}
+
+void main() {
+  canSelect = true;
+  doWhileLoop();
+
+  canSelect = false;
+  doWhileLoop();
+}