From 12b1773deba18ce53e7e2c53ef5b2d44e3e85476 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Wed, 17 Nov 2021 12:12:31 +0000 Subject: [PATCH] [vm] Fix issues with flutter_regress_91370_il_test * Add Canonicalize pass after CSE to erase graph differences between 64-bit and 32-bit platforms. These differences originate from different treatment of integers (we unbox more eagerly on 64-bit platform) and consequently affect load forwarding - which generates some redundant[1] phis on 64-bit platform, but not on 32-bit platform. Adding this pass also addresses other cases of unpredictable performance when IL changes can cause CSE not to trigger a Canonicalize inside itself and regress performance, by inhibiting subsequent optimizations. * Allow IL tests to check whether the graph was compiled in sound null safety mode or not - because graphs can have minor differences. * Use `"$i"` instead of `i.toString()` in the test to allow TFA to infer non-nullability of fields in non-sound-nullsafety mode. [1] The definition of redundant phi differs between canonicalization pass and load forwarding pass. The former unwraps redefinitions and the latter does not. We are going to address this difference in a more systematic way when we split input dependencies from control dependencies. TEST=ci Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-product-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-precomp-linux-release-simarm-try,vm-kernel-precomp-nnbd-linux-release-x64-try,vm-kernel-precomp-nnbd-linux-release-simarm_x64-try Change-Id: Ifbd68fbb7f3b374ecee8ada1b3a4a2249139a108 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/220541 Commit-Queue: Slava Egorov Reviewed-by: Daco Harkes --- pkg/vm/bin/compare_il.dart | 2 +- pkg/vm/lib/testing/il_matchers.dart | 36 +++++++++++++++++-- .../dart/flutter_regress_91370_il_test.dart | 36 ++++++++++++------- .../dart_2/flutter_regress_91370_il_test.dart | 36 ++++++++++++------- runtime/vm/compiler/backend/il_printer.cc | 3 ++ runtime/vm/compiler/compiler_pass.cc | 3 +- 6 files changed, 88 insertions(+), 28 deletions(-) diff --git a/pkg/vm/bin/compare_il.dart b/pkg/vm/bin/compare_il.dart index 8196a5e3074..5b9caa36862 100644 --- a/pkg/vm/bin/compare_il.dart +++ b/pkg/vm/bin/compare_il.dart @@ -138,7 +138,7 @@ Map> _loadGraphs(String ilFile, Renamer rename) { for (var graph in File(ilFile).readAsLinesSync()) { final m = jsonDecode(graph) as Map; graphs.putIfAbsent(m['f'], () => {})[m['p']] = - FlowGraph(m['b'], m['desc'], rename: rename); + FlowGraph(m['b'], m['desc'], m['flags'], rename: rename); } return graphs; diff --git a/pkg/vm/lib/testing/il_matchers.dart b/pkg/vm/lib/testing/il_matchers.dart index dd5eb8f56df..1c2f8835b03 100644 --- a/pkg/vm/lib/testing/il_matchers.dart +++ b/pkg/vm/lib/testing/il_matchers.dart @@ -12,14 +12,18 @@ typedef Renamer = String Function(String); class FlowGraph { final List blocks; final Map descriptors; + final Map flags; final Renamer rename; - FlowGraph(this.blocks, Map desc, {required this.rename}) + FlowGraph(this.blocks, Map desc, this.flags, + {required this.rename}) : descriptors = { for (var e in desc.entries) e.key: InstructionDescriptor.fromJson(e.value) }; + bool get soundNullSafety => flags['nnbd']; + /// Match the sequence of blocks in this flow graph against the given /// sequence of matchers: `expected[i]` is expected to match `blocks[i]`, /// but there can be more blocks in the graph than matchers (the suffix is @@ -33,11 +37,34 @@ class FlowGraph { env ??= Env(rename: rename, descriptors: descriptors); for (var i = 0; i < expected.length; i++) { - expected[i].match(env, blocks[i]).expectMatched('failed to match'); + final result = expected[i].match(env, blocks[i]); + if (result.isFail) { + print('Failed to match: ${result.message}'); + dump(); + throw 'Failed to match'; + } } return env; } + + void dump() { + for (var block in blocks) { + print('B${block['b']}[${block['o']}]'); + for (var instr in [...?block['d'], ...?block['is']]) { + final v = instr['v'] ?? -1; + final prefix = v != -1 ? 'v$v <- ' : ''; + final inputs = instr['i']?.map((v) => 'v$v').join(', ') ?? ''; + final attrs = descriptors[instr['o']] + ?.attributeIndex + .entries + .map((e) => '${e.key}: ${instr['d'][e.value]}') + .join(','); + final attrsWrapped = attrs != null ? '[$attrs]' : ''; + print(' ${prefix}${instr['o']}$attrsWrapped($inputs)'); + } + } + } } class InstructionDescriptor { @@ -304,6 +331,11 @@ class _AttributesMatcher implements Matcher { .toList()); return impl!.match(e, v); } + + @override + String toString() { + return matchers.toString(); + } } /// Matcher which matches an instruction with opcode [op] and properties diff --git a/runtime/tests/vm/dart/flutter_regress_91370_il_test.dart b/runtime/tests/vm/dart/flutter_regress_91370_il_test.dart index 722da155992..afa6bcf7a7e 100644 --- a/runtime/tests/vm/dart/flutter_regress_91370_il_test.dart +++ b/runtime/tests/vm/dart/flutter_regress_91370_il_test.dart @@ -221,8 +221,8 @@ void main(List args) { BImpl(A1("")).loadWithNamedParam(h: H(A1(""))); for (var i = 0; i < 2; i++) { - final a1 = A1(i.toString()); - final a0 = A0(i.toDouble(), i.toString()); + final a1 = A1("$i"); + final a0 = A0(i.toDouble(), "$i"); B1(a1).testNarrowingThroughThisCallWithPositionalParam(H(a1)); B0(a0).testNarrowingThroughThisCallWithPositionalParam(H(a0)); B1(a1).testNarrowingThroughThisCallWithNamedParams(H(a1)); @@ -601,7 +601,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( ]), 'B1' << match.block('Join', [ - 'a.data[0]*' << match.Phi('a.data[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -617,8 +616,13 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( match.block('Target', [ match.Redefinition('this'), // This redefinition was inserted by load forwarding. - 'a.data[0]**' << match.Redefinition('a.data[0]*'), - 'a.data[0].str' << match.LoadField('a.data[0]**', slot: 'str'), + 'a.data[0]*' << match.Redefinition('a.data[0]'), + if (!beforeLICM.soundNullSafety) + 'a.data[0]*!' << match.CheckNull('a.data[0]*'), + 'a.data[0].str' << + match.LoadField( + beforeLICM.soundNullSafety ? 'a.data[0]*' : 'a.data[0]*!', + slot: 'str'), ]), ]); @@ -632,7 +636,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( ]), 'B1' << match.block('Join', [ - 'a.data[0]*' << match.Phi('a.data[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -646,7 +649,12 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( ]), 'B3' << match.block('Target', [ - 'a.data[0].str' << match.LoadField('a.data[0]*', slot: 'str'), + if (!beforeLICM.soundNullSafety) + 'a.data[0]!' << match.CheckNull('a.data[0]'), + 'a.data[0].str' << + match.LoadField( + beforeLICM.soundNullSafety ? 'a.data[0]' : 'a.data[0]!', + slot: 'str'), ]), ], env: env); } @@ -668,7 +676,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( ]), 'B1' << match.block('Join', [ - 'a[0]*' << match.Phi('a[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -684,8 +691,11 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( match.block('Target', [ match.Redefinition('this'), // This redefinition was inserted by load forwarding. - 'a[0]**' << match.Redefinition('a[0]*'), - 'a[0].str' << match.LoadField('a[0]**', slot: 'str'), + 'a[0]*' << match.Redefinition('a[0]'), + if (!beforeLICM.soundNullSafety) 'a[0]*!' << match.CheckNull('a[0]*'), + 'a[0].str' << + match.LoadField(beforeLICM.soundNullSafety ? 'a[0]*' : 'a[0]*!', + slot: 'str'), ]), ]); @@ -698,7 +708,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( ]), 'B1' << match.block('Join', [ - 'a[0]*' << match.Phi('a[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -712,7 +721,10 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( ]), 'B3' << match.block('Target', [ - 'a[0].str' << match.LoadField('a[0]*', slot: 'str'), + if (!beforeLICM.soundNullSafety) 'a[0]!' << match.CheckNull('a[0]'), + 'a[0].str' << + match.LoadField(beforeLICM.soundNullSafety ? 'a[0]' : 'a[0]!', + slot: 'str'), ]), ], env: env); } diff --git a/runtime/tests/vm/dart_2/flutter_regress_91370_il_test.dart b/runtime/tests/vm/dart_2/flutter_regress_91370_il_test.dart index 722da155992..afa6bcf7a7e 100644 --- a/runtime/tests/vm/dart_2/flutter_regress_91370_il_test.dart +++ b/runtime/tests/vm/dart_2/flutter_regress_91370_il_test.dart @@ -221,8 +221,8 @@ void main(List args) { BImpl(A1("")).loadWithNamedParam(h: H(A1(""))); for (var i = 0; i < 2; i++) { - final a1 = A1(i.toString()); - final a0 = A0(i.toDouble(), i.toString()); + final a1 = A1("$i"); + final a0 = A0(i.toDouble(), "$i"); B1(a1).testNarrowingThroughThisCallWithPositionalParam(H(a1)); B0(a0).testNarrowingThroughThisCallWithPositionalParam(H(a0)); B1(a1).testNarrowingThroughThisCallWithNamedParams(H(a1)); @@ -601,7 +601,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( ]), 'B1' << match.block('Join', [ - 'a.data[0]*' << match.Phi('a.data[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -617,8 +616,13 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( match.block('Target', [ match.Redefinition('this'), // This redefinition was inserted by load forwarding. - 'a.data[0]**' << match.Redefinition('a.data[0]*'), - 'a.data[0].str' << match.LoadField('a.data[0]**', slot: 'str'), + 'a.data[0]*' << match.Redefinition('a.data[0]'), + if (!beforeLICM.soundNullSafety) + 'a.data[0]*!' << match.CheckNull('a.data[0]*'), + 'a.data[0].str' << + match.LoadField( + beforeLICM.soundNullSafety ? 'a.data[0]*' : 'a.data[0]*!', + slot: 'str'), ]), ]); @@ -632,7 +636,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( ]), 'B1' << match.block('Join', [ - 'a.data[0]*' << match.Phi('a.data[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -646,7 +649,12 @@ void matchIL$testNarrowingThroughIndexedLoadFromGrowableArray( ]), 'B3' << match.block('Target', [ - 'a.data[0].str' << match.LoadField('a.data[0]*', slot: 'str'), + if (!beforeLICM.soundNullSafety) + 'a.data[0]!' << match.CheckNull('a.data[0]'), + 'a.data[0].str' << + match.LoadField( + beforeLICM.soundNullSafety ? 'a.data[0]' : 'a.data[0]!', + slot: 'str'), ]), ], env: env); } @@ -668,7 +676,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( ]), 'B1' << match.block('Join', [ - 'a[0]*' << match.Phi('a[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -684,8 +691,11 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( match.block('Target', [ match.Redefinition('this'), // This redefinition was inserted by load forwarding. - 'a[0]**' << match.Redefinition('a[0]*'), - 'a[0].str' << match.LoadField('a[0]**', slot: 'str'), + 'a[0]*' << match.Redefinition('a[0]'), + if (!beforeLICM.soundNullSafety) 'a[0]*!' << match.CheckNull('a[0]*'), + 'a[0].str' << + match.LoadField(beforeLICM.soundNullSafety ? 'a[0]*' : 'a[0]*!', + slot: 'str'), ]), ]); @@ -698,7 +708,6 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( ]), 'B1' << match.block('Join', [ - 'a[0]*' << match.Phi('a[0]', match.any), match.CheckStackOverflow(), match.Branch(match.RelationalOp(match.any, match.any, kind: '<'), ifTrue: 'B2'), @@ -712,7 +721,10 @@ void matchIL$testNarrowingThroughIndexedLoadFromFixedArray( ]), 'B3' << match.block('Target', [ - 'a[0].str' << match.LoadField('a[0]*', slot: 'str'), + if (!beforeLICM.soundNullSafety) 'a[0]!' << match.CheckNull('a[0]'), + 'a[0].str' << + match.LoadField(beforeLICM.soundNullSafety ? 'a[0]' : 'a[0]!', + slot: 'str'), ]), ], env: env); } diff --git a/runtime/vm/compiler/backend/il_printer.cc b/runtime/vm/compiler/backend/il_printer.cc index df13a3a573b..f7b0d456d9c 100644 --- a/runtime/vm/compiler/backend/il_printer.cc +++ b/runtime/vm/compiler/backend/il_printer.cc @@ -44,6 +44,9 @@ class IlTestPrinter : public AllStatic { writer.OpenObject("desc"); AttributesSerializer(&writer).WriteDescriptors(); writer.CloseObject(); + writer.OpenObject("flags"); + writer.PrintPropertyBool("nnbd", IsolateGroup::Current()->null_safety()); + writer.CloseObject(); writer.CloseObject(); THR_Print("%s\n", writer.ToCString()); } diff --git a/runtime/vm/compiler/compiler_pass.cc b/runtime/vm/compiler/compiler_pass.cc index 4bbce2c2fec..8e34d21e976 100644 --- a/runtime/vm/compiler/compiler_pass.cc +++ b/runtime/vm/compiler/compiler_pass.cc @@ -357,6 +357,7 @@ FlowGraph* CompilerPass::RunPipeline(PipelineMode mode, INVOKE_PASS(WidenSmiToInt32); INVOKE_PASS(SelectRepresentations); INVOKE_PASS(CSE); + INVOKE_PASS(Canonicalize); INVOKE_PASS(LICM); INVOKE_PASS(TryOptimizePatterns); INVOKE_PASS(DSE); @@ -387,6 +388,7 @@ FlowGraph* CompilerPass::RunPipeline(PipelineMode mode, INVOKE_PASS(AllocationSinking_DetachMaterializations); INVOKE_PASS(EliminateWriteBarriers); INVOKE_PASS(FinalizeGraph); + INVOKE_PASS(Canonicalize); INVOKE_PASS(AllocateRegisters); INVOKE_PASS(ReorderBlocks); return pass_state->flow_graph(); @@ -573,7 +575,6 @@ COMPILER_PASS(FinalizeGraph, { flow_graph->function().set_inlining_depth(state->inlining_depth); // Remove redefinitions for the rest of the pipeline. flow_graph->RemoveRedefinitions(); - flow_graph->Canonicalize(); }); COMPILER_PASS(GenerateCode, { state->graph_compiler->CompileGraph(); });