languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-09-18 21:21:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

[vm] Fix issues with flutter_regress_91370_il_test

Browse source 
* 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 <vegorov@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
This commit is contained in:
Vyacheslav Egorov 2021-11-17 12:12:31 +00:00 committed by commit-bot@chromium.org
parent bfbcfa6dc5
commit 12b1773deb
6 changed files with 88 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
pkg/vm/bin/compare_il.dart
View file

@ -138,7 +138,7 @@ Map<String, Map<String, FlowGraph>> _loadGraphs(String ilFile, Renamer rename) {
for (var graph in File(ilFile).readAsLinesSync()) {
final m = jsonDecode(graph) as Map<String, dynamic>;
graphs.putIfAbsent(m['f'], () => {})[m['p']] =
FlowGraph(m['b'], m['desc'], rename: rename);
FlowGraph(m['b'], m['desc'], m['flags'], rename: rename);
}
return graphs;

36
pkg/vm/lib/testing/il_matchers.dart
View file

@ -12,14 +12,18 @@ typedef Renamer = String Function(String);
class FlowGraph {
final List<dynamic> blocks;
final Map<String, InstructionDescriptor> descriptors;
final Map<String, dynamic> flags;
final Renamer rename;
FlowGraph(this.blocks, Map<String, dynamic> desc, {required this.rename})
FlowGraph(this.blocks, Map<String, dynamic> 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

36
runtime/tests/vm/dart/flutter_regress_91370_il_test.dart
View file

@ -221,8 +221,8 @@ void main(List<String> 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);
}

36
runtime/tests/vm/dart_2/flutter_regress_91370_il_test.dart
View file

@ -221,8 +221,8 @@ void main(List<String> 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);
}

3
runtime/vm/compiler/backend/il_printer.cc
View file

@ -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());
}

3
runtime/vm/compiler/compiler_pass.cc
View file

@ -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(); });