diff --git a/runtime/tests/vm/dart/regress_flutter98967_test.dart b/runtime/tests/vm/dart/regress_flutter98967_test.dart new file mode 100644 index 00000000000..97feb2dd41d --- /dev/null +++ b/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(); +} diff --git a/runtime/vm/compiler/backend/flow_graph.h b/runtime/vm/compiler/backend/flow_graph.h index 2c82c131175..93c832a767b 100644 --- a/runtime/vm/compiler/backend/flow_graph.h +++ b/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_; diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index 8991d8d69ce..256e184b303 100644 --- a/runtime/vm/compiler/backend/il.cc +++ b/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); } } diff --git a/runtime/vm/compiler/backend/il_test.cc b/runtime/vm/compiler/backend/il_test.cc index 30502a42f19..d29658c16ed 100644 --- a/runtime/vm/compiler/backend/il_test.cc +++ b/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()) { diff --git a/runtime/vm/compiler/compiler_pass.cc b/runtime/vm/compiler/compiler_pass.cc index 1faa8d39b8b..33079e7a374 100644 --- a/runtime/vm/compiler/compiler_pass.cc +++ b/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(); }); diff --git a/runtime/vm/compiler/compiler_pass.h b/runtime/vm/compiler/compiler_pass.h index 01a71949204..bac28afa8ab 100644 --- a/runtime/vm/compiler/compiler_pass.h +++ b/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) \