[vm,compiler] Fix incorrect handling of unwrapped phi in constant propagation

When constant propagation evaluates EqualityCompare or StrictCompare,
it can unwrap phis (if they are redundant wrt current set of reachable
blocks), and calculate value if both lhs and rhs operands are the same.

Previously, phi was marked as unwrapped only if that calculation changes
the constant value of comparison.

This is not correct as calculation based on unwrapped phi
needs to be invalidated anyway if more reachable blocks are discovered
and unwrapped phi becomes non-redundant.

The fix is to mark the unwrapped phis regardless of whether the constant
value of comparison has been changed or not.

TEST=vm/dart/regress_60349_test

Fixes #60349

Change-Id: I01e46d6def67fdc5d4c0bdc3a73aaac94725e221
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417021
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>

Author: alexmarkov

runtime/tests/vm/dart/regress_60349_test.dart

@@ -0,0 +1,54 @@
+// Copyright (c) 2025, 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.
+
+// VMOptions=--optimization_counter_threshold=100 --deterministic
+
+// Verify that constant propagation doesn't perform an incorrect
+// optimization involving unwrapped phi.
+// Regression test for https://github.com/dart-lang/sdk/issues/60349
+
+import "package:expect/expect.dart";
+
+@pragma('vm:never-inline')
+void foo(int? x, int y) {
+  List<int> l = [1];
+  int ll = l.length; // ll = 1
+  int n = 0;
+  int? a;
+  while (true) {
+    int b = 0;
+    // As ll == 1, the loop below only ever has one iteration.
+    for (int i = 0; i < ll; i++) {
+      // y == 2, and n only reaches 1 in this program, but removing this
+      // condition makes the bug go away:
+      if (n == y) {
+        return;
+      }
+      b = 1234 + n * 1111;
+    }
+
+    // On the first iteration, "a" is set to "b" (2025).
+    if (n == 0) {
+      a = b;
+    }
+
+    // This condition is false on the first iteration (1234 vs 2025) and true
+    // on the second (2345 vs 2025):
+    if (b >= x!) {
+      // This should never be true.
+      if (b == a) {
+        Expect.fail("BUG! $b == $a?!?");
+      }
+      return;
+    }
+
+    n++;
+  }
+}
+
+void main() {
+  for (int i = 0; i < 150; ++i) {
+    foo(int.parse('2025'), int.parse('2'));
+  }
+}

runtime/vm/compiler/backend/constant_propagator.cc

@@ -586,13 +586,12 @@ void ConstantPropagator::VisitStrictCompare(StrictCompareInstr* instr) {
   Definition* unwrapped_right_defn = UnwrapPhi(right_defn);
   if (unwrapped_left_defn == unwrapped_right_defn) {
     // Fold x === x, and x !== x to true/false.
-    if (SetValue(instr, Bool::Get(instr->kind() == Token::kEQ_STRICT))) {
-      if (unwrapped_left_defn != left_defn) {
-        MarkUnwrappedPhi(left_defn);
-      }
-      if (unwrapped_right_defn != right_defn) {
-        MarkUnwrappedPhi(right_defn);
-      }
+    SetValue(instr, Bool::Get(instr->kind() == Token::kEQ_STRICT));
+    if (unwrapped_left_defn != left_defn) {
+      MarkUnwrappedPhi(left_defn);
+    }
+    if (unwrapped_right_defn != right_defn) {
+      MarkUnwrappedPhi(right_defn);
     }
     return;
   }
@@ -711,13 +710,12 @@ void ConstantPropagator::VisitEqualityCompare(EqualityCompareInstr* instr) {
     Definition* unwrapped_right_defn = UnwrapPhi(right_defn);
     if (unwrapped_left_defn == unwrapped_right_defn) {
       // Fold x === x, and x !== x to true/false.
-      if (SetValue(instr, Bool::Get(instr->kind() == Token::kEQ))) {
-        if (unwrapped_left_defn != left_defn) {
-          MarkUnwrappedPhi(left_defn);
-        }
-        if (unwrapped_right_defn != right_defn) {
-          MarkUnwrappedPhi(right_defn);
-        }
+      SetValue(instr, Bool::Get(instr->kind() == Token::kEQ));
+      if (unwrapped_left_defn != left_defn) {
+        MarkUnwrappedPhi(left_defn);
+      }
+      if (unwrapped_right_defn != right_defn) {
+        MarkUnwrappedPhi(right_defn);
       }
       return;
     }