From 736cec66d95bffaa080e6ea753dab01b022f1b6c Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Fri, 25 Feb 2022 18:36:02 +0000 Subject: [PATCH] [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 Reviewed-by: Martin Kustermann Commit-Queue: Alexander Markov --- .../vm/dart/regress_flutter98967_test.dart | 53 +++++++++++++++++++ runtime/vm/compiler/backend/flow_graph.h | 12 +++++ runtime/vm/compiler/backend/il.cc | 6 ++- runtime/vm/compiler/backend/il_test.cc | 43 +++++++++++++++ runtime/vm/compiler/compiler_pass.cc | 11 +++- runtime/vm/compiler/compiler_pass.h | 1 + 6 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 runtime/tests/vm/dart/regress_flutter98967_test.dart 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) \