[vm/compiler] Avoid unmatched representations after the last SelectRepresentations pass

Before this fix, Canonicalize pass could change representation
of inputs of EqualityCompare after the last SelectRepresentations
pass. This results in unmatched representations and invalid code
generated.

The fix is to disallow canonicalization of EqualityCompare
from null-aware to non-null-aware after the last SelectRepresentations.

TEST=vm/dart/regress_flutter98967_test
Fixes https://github.com/flutter/flutter/issues/98967

Change-Id: I05359737fe322fbb2a0fe6025e3716ba5d04ebbf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/234324
Reviewed-by: Slava Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>

runtime/tests/vm/dart/regress_flutter98967_test.dart

@@ -0,0 +1,53 @@
+// Copyright (c) 2022, 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.
+
+// Regression test for https://github.com/flutter/flutter/issues/98967.
+// Verifies that compiler doesn't generate wrong code for comparison of ints
+// due to a late change in the representation of EqualityCompare inputs.
+
+import 'package:expect/expect.dart';
+
+class C {
+  int? val;
+
+  @pragma('vm:never-inline')
+  void testImpl(bool Function(int) compare) {
+    for (var i = 0; i < 2; i++) {
+      Expect.equals(false, compare(i));
+      val = i;
+      Expect.equals(true, compare(i));
+    }
+
+    final mint0 = int.parse("7fffffffffffffff", radix: 16);
+    final mint1 = int.parse("7fffffffffffffff", radix: 16);
+    if (mint0 != mint1) throw 'This is the same mint value';
+
+    Expect.equals(false, compare(mint0));
+    val = mint0;
+    Expect.equals(true, compare(mint0));
+    Expect.equals(true, compare(mint1),
+        'expected two different mints with the same value compare equal');
+  }
+
+  @pragma('vm:never-inline')
+  static void blackhole(void Function() f) {
+    f();
+  }
+
+  void test() {
+    return testImpl((v) {
+      // Note: need multiple context levels in the chain to delay
+      // optimizer forwarding load of [val] and subsequently
+      // clearing null_aware flag on the equality comparison.
+      // Hence the closure capturing [v] below.
+      final result = val != null ? val == v : false;
+      blackhole(() => v);
+      return result;
+    });
+  }
+}
+
+void main() {
+  C().test();
+}

runtime/vm/compiler/backend/flow_graph.h

@@ -397,6 +397,17 @@ class FlowGraph : public ZoneAllocated {
   // after this point.
   void disallow_licm() { licm_allowed_ = false; }
 
+  // Returns true if mismatch in input/output representations is allowed.
+  bool unmatched_representations_allowed() const {
+    return unmatched_representations_allowed_;
+  }
+
+  // After the last SelectRepresentations pass all further transformations
+  // should maintain matching input/output representations.
+  void disallow_unmatched_representations() {
+    unmatched_representations_allowed_ = false;
+  }
+
   PrologueInfo prologue_info() const { return prologue_info_; }
 
   // Computes the loop hierarchy of the flow graph on demand.
@@ -623,6 +634,7 @@ class FlowGraph : public ZoneAllocated {
   ConstantInstr* constant_dead_;
 
   bool licm_allowed_;
+  bool unmatched_representations_allowed_ = true;
 
   const PrologueInfo prologue_info_;
 

runtime/vm/compiler/backend/il.cc

@@ -3460,7 +3460,11 @@ Definition* EqualityCompareInstr::Canonicalize(FlowGraph* flow_graph) {
       flow_graph->InsertBefore(this, replacement, env(), FlowGraph::kValue);
       return replacement;
     } else {
-      if (!left_type->is_nullable() && !right_type->is_nullable()) {
+      // Null-aware EqualityCompare takes boxed inputs, so make sure
+      // unmatched representations are still allowed when converting
+      // EqualityCompare to the unboxed instruction.
+      if (!left_type->is_nullable() && !right_type->is_nullable() &&
+          flow_graph->unmatched_representations_allowed()) {
         set_null_aware(false);
       }
     }

runtime/vm/compiler/backend/il_test.cc

@@ -390,6 +390,49 @@ static void WriteCidTo(intptr_t cid, BaseTextBuffer* buffer) {
   }
 }
 
+static void TestNullAwareEqualityCompareCanonicalization(
+    Thread* thread,
+    bool allow_representation_change) {
+  using compiler::BlockBuilder;
+
+  CompilerState S(thread, /*is_aot=*/true, /*is_optimizing=*/true);
+
+  FlowGraphBuilderHelper H;
+
+  auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
+
+  EqualityCompareInstr* compare = nullptr;
+  {
+    BlockBuilder builder(H.flow_graph(), normal_entry);
+    Definition* v0 =
+        builder.AddParameter(0, 0, /*with_frame=*/true, kUnboxedInt64);
+    Definition* v1 =
+        builder.AddParameter(1, 1, /*with_frame=*/true, kUnboxedInt64);
+    Definition* box0 = builder.AddDefinition(new BoxInt64Instr(new Value(v0)));
+    Definition* box1 = builder.AddDefinition(new BoxInt64Instr(new Value(v1)));
+
+    compare = builder.AddDefinition(new EqualityCompareInstr(
+        InstructionSource(), Token::kEQ, new Value(box0), new Value(box1),
+        kMintCid, S.GetNextDeoptId(), /*null_aware=*/true));
+    builder.AddReturn(new Value(compare));
+  }
+
+  H.FinishGraph();
+
+  if (!allow_representation_change) {
+    H.flow_graph()->disallow_unmatched_representations();
+  }
+
+  H.flow_graph()->Canonicalize();
+
+  EXPECT(compare->is_null_aware() == !allow_representation_change);
+}
+
+ISOLATE_UNIT_TEST_CASE(IL_Canonicalize_EqualityCompare) {
+  TestNullAwareEqualityCompareCanonicalization(thread, true);
+  TestNullAwareEqualityCompareCanonicalization(thread, false);
+}
+
 static void WriteCidRangeVectorTo(const CidRangeVector& ranges,
                                   BaseTextBuffer* buffer) {
   if (ranges.is_empty()) {

runtime/vm/compiler/compiler_pass.cc

@@ -309,7 +309,7 @@ FlowGraph* CompilerPass::RunForceOptimizedPipeline(
   INVOKE_PASS(ConstantPropagation);
   INVOKE_PASS(TypePropagation);
   INVOKE_PASS(WidenSmiToInt32);
-  INVOKE_PASS(SelectRepresentations);
+  INVOKE_PASS(SelectRepresentations_Final);
   INVOKE_PASS(TypePropagation);
   INVOKE_PASS(TryCatchOptimization);
   INVOKE_PASS(EliminateEnvironments);
@@ -380,7 +380,7 @@ FlowGraph* CompilerPass::RunPipeline(PipelineMode mode,
   INVOKE_PASS(EliminateDeadPhis);
   INVOKE_PASS(DCE);
   INVOKE_PASS(TypePropagation);
-  INVOKE_PASS(SelectRepresentations);
+  INVOKE_PASS(SelectRepresentations_Final);
   INVOKE_PASS(Canonicalize);
   INVOKE_PASS(UseTableDispatch);
   INVOKE_PASS(EliminateStackOverflowChecks);
@@ -469,6 +469,13 @@ COMPILER_PASS(SelectRepresentations, {
   flow_graph->SelectRepresentations();
 });
 
+COMPILER_PASS(SelectRepresentations_Final, {
+  // Final selection of representations. After this pass
+  // representations of inputs/outputs should match.
+  flow_graph->SelectRepresentations();
+  flow_graph->disallow_unmatched_representations();
+});
+
 COMPILER_PASS(UseTableDispatch, {
   state->call_specializer->ReplaceInstanceCallsWithDispatchTableCalls();
 });

runtime/vm/compiler/compiler_pass.h

@@ -46,6 +46,7 @@ namespace dart {
   V(RangeAnalysis)                                                             \
   V(ReorderBlocks)                                                             \
   V(SelectRepresentations)                                                     \
+  V(SelectRepresentations_Final)                                               \
   V(SetOuterInliningId)                                                        \
   V(TryCatchOptimization)                                                      \
   V(TryOptimizePatterns)                                                       \