From 397ed8673c8dfc702f136a79bcbccac40efa3369 Mon Sep 17 00:00:00 2001 From: Mayank Patke Date: Tue, 7 Feb 2023 20:07:57 +0000 Subject: [PATCH] [dart2js] Add phase 1 kernel transformer to simplify const conditionals. Suppose we have code like if (isDevMode) { // do foo } else { // do bar } where isDevMode is const (or can be evaluated as const). In particular, isDevMode can be controlled by compile-time options, like bool.fromEnvironment. We currently eliminate the dead branch during SSA, but that means we do the work of compiling dead code in the previous phases. Instead, we can recognize that the condition is const (or effectively const) and eliminate the dead AST subtree directly. Change-Id: Ia91da5ebc7fa496a1b963308c6e02d572cab936e Bug: b/254543452 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/270281 Reviewed-by: Sigmund Cherem --- pkg/compiler/lib/src/compiler.dart | 20 ++++- .../const_conditional_simplifier.dart | 75 +++++++++++++++++++ .../test/codegen/load_elimination_test.dart | 2 +- pkg/compiler/test/impact/data/statements.dart | 65 ++++++++++++++-- pkg/compiler/test/inference/data/general.dart | 45 ++++++----- .../test/inference/data/refine_order.dart | 7 +- pkg/compiler/test/model/native_test.dart | 2 + tests/web/jsinterop_test.dart | 4 +- tests/web/non_jsinterop_test.dart | 4 +- tests/web_2/jsinterop_test.dart | 4 +- tests/web_2/non_jsinterop_test.dart | 4 +- 11 files changed, 195 insertions(+), 37 deletions(-) create mode 100644 pkg/compiler/lib/src/kernel/transformations/const_conditional_simplifier.dart diff --git a/pkg/compiler/lib/src/compiler.dart b/pkg/compiler/lib/src/compiler.dart index 56716a979a4..0d3d0ac89e9 100644 --- a/pkg/compiler/lib/src/compiler.dart +++ b/pkg/compiler/lib/src/compiler.dart @@ -52,6 +52,7 @@ import 'js_model/locals.dart'; import 'kernel/front_end_adapter.dart' show CompilerFileSystem; import 'kernel/kernel_strategy.dart'; import 'kernel/kernel_world.dart'; +import 'kernel/transformations/const_conditional_simplifier.dart'; import 'null_compiler_output.dart' show NullCompilerOutput; import 'options.dart' show CompilerOptions; import 'phase/load_kernel.dart' as load_kernel; @@ -442,6 +443,19 @@ class Compiler { bool shouldStopAfterLoadKernel(load_kernel.Output? output) => output == null || compilationFailed || options.cfeOnly; + void simplifyConstConditionals(load_kernel.Output output) { + if (options.readClosedWorldUri == null) { + // No existing closed world means we're in phase 1, so run the + // transformer. + ConstConditionalSimplifier( + output.component, environment, reporter, options) + .run(); + } + + // If the closed world is deserialized instead, then the input .dill should + // already have the modified AST. + } + Future runModularAnalysis( load_kernel.Output output, Set moduleLibraries) async { ir.Component component = output.component; @@ -685,17 +699,19 @@ class Compiler { final output = await produceKernel(); if (shouldStopAfterLoadKernel(output)) return; + simplifyConstConditionals(output!); + // Run modular analysis. This may be null if modular analysis was not // requested for this pipeline. ModuleData? moduleData; if (options.modularMode || options.hasModularAnalysisInputs) { - moduleData = await produceModuleData(output!); + moduleData = await produceModuleData(output); } if (shouldStopAfterModularAnalysis) return; // Compute closed world. DataAndIndices? closedWorldAndIndices = - await produceClosedWorld(output!, moduleData); + await produceClosedWorld(output, moduleData); if (shouldStopAfterClosedWorld(closedWorldAndIndices)) return; // Run global analysis. diff --git a/pkg/compiler/lib/src/kernel/transformations/const_conditional_simplifier.dart b/pkg/compiler/lib/src/kernel/transformations/const_conditional_simplifier.dart new file mode 100644 index 00000000000..28556b8cb20 --- /dev/null +++ b/pkg/compiler/lib/src/kernel/transformations/const_conditional_simplifier.dart @@ -0,0 +1,75 @@ +// Copyright (c) 2023, 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. + +import 'package:front_end/src/api_prototype/constant_evaluator.dart'; +import 'package:front_end/src/api_unstable/dart2js.dart' show LocatedMessage; +import 'package:kernel/ast.dart'; +import 'package:kernel/class_hierarchy.dart'; +import 'package:kernel/core_types.dart'; +import 'package:kernel/type_environment.dart'; + +import '../../diagnostics/diagnostic_listener.dart'; +import '../../environment.dart'; +import '../../ir/constants.dart'; +import '../../kernel/element_map.dart'; +import '../../options.dart'; + +class ConstConditionalSimplifier extends RemovingTransformer { + final Component _component; + final Environment _environment; + final DiagnosticReporter _reporter; + final CompilerOptions _options; + + late final TypeEnvironment _typeEnvironment; + late final Dart2jsConstantEvaluator _constantEvaluator; + late StaticTypeContext _staticTypeContext; + + ConstConditionalSimplifier( + this._component, this._environment, this._reporter, this._options) { + final coreTypes = CoreTypes(_component); + final classHierarchy = ClassHierarchy(_component, coreTypes); + _typeEnvironment = TypeEnvironment(coreTypes, classHierarchy); + _constantEvaluator = Dart2jsConstantEvaluator(_component, _typeEnvironment, + (LocatedMessage message, List? context) { + reportLocatedMessage(_reporter, message, context); + }, + environment: _environment, + evaluationMode: _options.useLegacySubtyping + ? EvaluationMode.weak + : EvaluationMode.strong); + } + + TreeNode run() => transform(_component); + + @override + TreeNode defaultMember(Member node, TreeNode? removalSentinel) { + _staticTypeContext = StaticTypeContext(node, _typeEnvironment); + return super.defaultMember(node, removalSentinel); + } + + @override + TreeNode visitConditionalExpression( + ConditionalExpression node, TreeNode? removalSentinel) { + super.visitConditionalExpression(node, removalSentinel); + final condition = _constantEvaluator.evaluateOrNull( + _staticTypeContext, node.condition, + requireConstant: false); + if (condition is! BoolConstant) return node; + return condition.value ? node.then : node.otherwise; + } + + @override + TreeNode visitIfStatement(IfStatement node, TreeNode? removalSentinel) { + super.visitIfStatement(node, removalSentinel); + final condition = _constantEvaluator.evaluateOrNull( + _staticTypeContext, node.condition, + requireConstant: false); + if (condition is! BoolConstant) return node; + if (condition.value) { + return node.then; + } else { + return node.otherwise ?? removalSentinel ?? EmptyStatement(); + } + } +} diff --git a/pkg/compiler/test/codegen/load_elimination_test.dart b/pkg/compiler/test/codegen/load_elimination_test.dart index 822a5821573..2dc31299830 100644 --- a/pkg/compiler/test/codegen/load_elimination_test.dart +++ b/pkg/compiler/test/codegen/load_elimination_test.dart @@ -260,7 +260,7 @@ main() { await test(TEST_12, 'return 6'); await test(TEST_13, 'return 6'); await test(TEST_14, 'return t1[0]'); - await test(TEST_15, 'return 42'); + await test(TEST_15, 'return \$.a = 42'); await test(TEST_16, 'return \$.a'); await test(TEST_17, RegExp(r'return (t1|\$\.x === 0 \? \$\.a = true : \$\.a = false);')); diff --git a/pkg/compiler/test/impact/data/statements.dart b/pkg/compiler/test/impact/data/statements.dart index 0685c849331..6fe174ccaaa 100644 --- a/pkg/compiler/test/impact/data/statements.dart +++ b/pkg/compiler/test/impact/data/statements.dart @@ -33,33 +33,82 @@ main() { testAssertWithMessage(); } -/*member: testIfThen:type=[ +/*member: flag: + static=[ + Rti._bind(1), + Rti._eval(1), + _arrayInstanceType(1), + _asBool(1), + _asBoolQ(1), + _asBoolS(1), + _asDouble(1), + _asDoubleQ(1), + _asDoubleS(1), + _asInt(1), + _asIntQ(1), + _asIntS(1), + _asNum(1), + _asNumQ(1), + _asNumS(1), + _asObject(1), + _asString(1), + _asStringQ(1), + _asStringS(1), + _asTop(1), + _generalAsCheckImplementation(1), + _generalIsTestImplementation(1), + _generalNullableAsCheckImplementation(1), + _generalNullableIsTestImplementation(1), + _installSpecializedAsCheck(1), + _installSpecializedIsTest(1), + _instanceType(1), + _isBool(1), + _isInt(1), + _isNum(1), + _isObject(1), + _isString(1), + _isTop(1), + findType(1), + instanceType(1)], + type=[ + inst:Closure, + inst:JSBool, + inst:JSNull, + param:bool*] +*/ +bool flag; // late + +/*member: testIfThen: + static=[set:flag], + type=[ inst:JSBool, inst:JSInt, inst:JSNumNotInt, inst:JSNumber, inst:JSPositiveInt, inst:JSUInt31, - inst:JSUInt32]*/ + inst:JSUInt32] +*/ testIfThen() { - // ignore: DEAD_CODE - if (false) return 42; + if (flag = false) return 42; return 1; } -/*member: testIfThenElse:type=[ +/*member: testIfThenElse: + static=[set:flag], + type=[ inst:JSBool, inst:JSInt, inst:JSNumNotInt, inst:JSNumber, inst:JSPositiveInt, inst:JSUInt31, - inst:JSUInt32]*/ + inst:JSUInt32] +*/ testIfThenElse() { - if (true) + if (flag = true) return 42; else - // ignore: DEAD_CODE return 1; } diff --git a/pkg/compiler/test/inference/data/general.dart b/pkg/compiler/test/inference/data/general.dart index 169314117c4..32cc9b16c9f 100644 --- a/pkg/compiler/test/inference/data/general.dart +++ b/pkg/compiler/test/inference/data/general.dart @@ -560,12 +560,15 @@ testDoWhile2() { return a; } +/*member: flag:Value([null|exact=JSBool], value: true)*/ +bool flag; // late + /*member: testDoWhile3:Value([exact=JSBool], value: false)*/ testDoWhile3() { dynamic a = 42; do { a = 'foo'; - if (true) continue; + if (flag = true) continue; return false; } while (true); // ignore: dead_code @@ -577,7 +580,7 @@ testDoWhile4() { dynamic a = 'foo'; do { a = 54; - if (true) break; + if (flag = true) break; return 3.5; } while (true); return a; @@ -686,11 +689,12 @@ class A { set myField(/*[subclass=JSUInt32]*/ a) {} /*member: A.returnInt1:[subclass=JSUInt32]*/ - returnInt1() => /*invoke: [exact=JSUInt31]*/ ++ /*[subclass=A]*/ /*update: [subclass=A]*/ myField; + returnInt1() => /*invoke: [exact=JSUInt31]*/ + ++ /*[subclass=A]*/ /*update: [subclass=A]*/ myField; /*member: A.returnInt2:[subclass=JSUInt32]*/ - returnInt2() => /*invoke: [exact=JSUInt31]*/ ++this - . /*[subclass=A]*/ /*update: [subclass=A]*/ myField; + returnInt2() => /*invoke: [exact=JSUInt31]*/ + ++this. /*[subclass=A]*/ /*update: [subclass=A]*/ myField; /*member: A.returnInt3:[subclass=JSUInt32]*/ returnInt3() => @@ -698,8 +702,8 @@ class A { 42; /*member: A.returnInt4:[subclass=JSUInt32]*/ - returnInt4() => /*[subclass=A]*/ /*update: [subclass=A]*/ myField /*invoke: [exact=JSUInt31]*/ += - 42; + returnInt4() => /*[subclass=A]*/ /*update: [subclass=A]*/ + myField /*invoke: [exact=JSUInt31]*/ += 42; /*member: A.[]:[exact=JSUInt31]*/ operator [](/*[exact=JSUInt31]*/ index) => 42; @@ -708,8 +712,8 @@ class A { operator []=(/*[exact=JSUInt31]*/ index, /*[subclass=JSUInt32]*/ value) {} /*member: A.returnInt5:[subclass=JSUInt32]*/ - returnInt5() => /*invoke: [exact=JSUInt31]*/ ++this /*[subclass=A]*/ /*update: [subclass=A]*/ [ - 0]; + returnInt5() => /*invoke: [exact=JSUInt31]*/ + ++this /*[subclass=A]*/ /*update: [subclass=A]*/ [0]; /*member: A.returnInt6:[subclass=JSUInt32]*/ returnInt6() => this /*[subclass=A]*/ /*update: [subclass=A]*/ [ @@ -724,16 +728,16 @@ class B extends A { B() : super.generative(); /*member: B.returnInt1:[subclass=JSUInt32]*/ - returnInt1() => /*invoke: [exact=JSUInt31]*/ ++new A() - . /*[exact=A]*/ /*update: [exact=A]*/ myField; + returnInt1() => /*invoke: [exact=JSUInt31]*/ + ++new A(). /*[exact=A]*/ /*update: [exact=A]*/ myField; /*member: B.returnInt2:[subclass=JSUInt32]*/ returnInt2() => new A() . /*[exact=A]*/ /*update: [exact=A]*/ myField /*invoke: [exact=JSUInt31]*/ += 4; /*member: B.returnInt3:[subclass=JSUInt32]*/ - returnInt3() => /*invoke: [exact=JSUInt31]*/ ++new A() /*[exact=A]*/ /*update: [exact=A]*/ [ - 0]; + returnInt3() => /*invoke: [exact=JSUInt31]*/ + ++new A() /*[exact=A]*/ /*update: [exact=A]*/ [0]; /*member: B.returnInt4:[subclass=JSUInt32]*/ returnInt4() => new A() /*[exact=A]*/ /*update: [exact=A]*/ [ @@ -763,11 +767,12 @@ class C { C(); /*member: C.returnInt1:[subclass=JSPositiveInt]*/ - returnInt1() => /*invoke: [subclass=JSPositiveInt]*/ ++ /*update: [exact=C]*/ /*[exact=C]*/ myField; + returnInt1() => /*invoke: [subclass=JSPositiveInt]*/ + ++ /*update: [exact=C]*/ /*[exact=C]*/ myField; /*member: C.returnInt2:[subclass=JSPositiveInt]*/ - returnInt2() => /*invoke: [subclass=JSPositiveInt]*/ ++this - . /*[exact=C]*/ /*update: [exact=C]*/ myField; + returnInt2() => /*invoke: [subclass=JSPositiveInt]*/ + ++this. /*[exact=C]*/ /*update: [exact=C]*/ myField; /*member: C.returnInt3:[subclass=JSPositiveInt]*/ returnInt3() => @@ -775,8 +780,8 @@ class C { 42; /*member: C.returnInt4:[subclass=JSPositiveInt]*/ - returnInt4() => /*[exact=C]*/ /*update: [exact=C]*/ myField /*invoke: [subclass=JSPositiveInt]*/ += - 42; + returnInt4() => /*[exact=C]*/ /*update: [exact=C]*/ + myField /*invoke: [subclass=JSPositiveInt]*/ += 42; /*member: C.[]:[subclass=JSPositiveInt]*/ operator [](/*[exact=JSUInt31]*/ index) => /*[exact=C]*/ myField; @@ -786,8 +791,8 @@ class C { /*[exact=JSUInt31]*/ index, /*[subclass=JSPositiveInt]*/ value) {} /*member: C.returnInt5:[subclass=JSPositiveInt]*/ - returnInt5() => /*invoke: [subclass=JSPositiveInt]*/ ++this /*[exact=C]*/ /*update: [exact=C]*/ [ - 0]; + returnInt5() => /*invoke: [subclass=JSPositiveInt]*/ + ++this /*[exact=C]*/ /*update: [exact=C]*/ [0]; /*member: C.returnInt6:[subclass=JSPositiveInt]*/ returnInt6() => this /*[exact=C]*/ /*update: [exact=C]*/ [ diff --git a/pkg/compiler/test/inference/data/refine_order.dart b/pkg/compiler/test/inference/data/refine_order.dart index cef03af5e79..73d124b0a62 100644 --- a/pkg/compiler/test/inference/data/refine_order.dart +++ b/pkg/compiler/test/inference/data/refine_order.dart @@ -136,11 +136,14 @@ conditionalCondition(/*[null|subclass=Object]*/ o) { // Access both branches of a conditional expression. //////////////////////////////////////////////////////////////////////////////// +/*member: flag:Value([null|exact=JSBool], value: true)*/ +bool flag; // late + /*member: conditionalBothBranches:[null]*/ @pragma('dart2js:assumeDynamic') conditionalBothBranches(/*[null|subclass=Object]*/ o) { // ignore: DEAD_CODE - true ? o.field : o.field; + (flag = true) ? o.field : o.field; o. /*[subclass=Object]*/ field; } @@ -152,7 +155,7 @@ conditionalBothBranches(/*[null|subclass=Object]*/ o) { @pragma('dart2js:assumeDynamic') conditionalOneBranchOnly(/*[null|subclass=Object]*/ o) { // ignore: DEAD_CODE - true ? o.field : null; + (flag = true) ? o.field : null; o.field; o. /*[subclass=Object]*/ field; } diff --git a/pkg/compiler/test/model/native_test.dart b/pkg/compiler/test/model/native_test.dart index 09ee5edbc95..34660a10490 100644 --- a/pkg/compiler/test/model/native_test.dart +++ b/pkg/compiler/test/model/native_test.dart @@ -21,6 +21,7 @@ main() { await runTest('tests/web_2/jsinterop_test.dart', '', { 'Class': Kind.regular, 'JsInteropClass': Kind.jsInterop, + 'flag': Kind.regular, 'topLevelField': Kind.regular, 'topLevelGetter': Kind.regular, 'topLevelSetter': Kind.regular, @@ -81,6 +82,7 @@ main() { await runTest('tests/web_2/non_jsinterop_test.dart', '', { 'Class': Kind.regular, 'JsInteropClass': Kind.jsInterop, + 'flag': Kind.regular, 'topLevelField': Kind.regular, 'topLevelGetter': Kind.regular, 'topLevelSetter': Kind.regular, diff --git a/tests/web/jsinterop_test.dart b/tests/web/jsinterop_test.dart index 994f1f1df55..9647186e423 100644 --- a/tests/web/jsinterop_test.dart +++ b/tests/web/jsinterop_test.dart @@ -239,8 +239,10 @@ class JsInteropClass { external static externalStaticJsInteropMethod(); } +late bool flag; + main() { - if (false) { + if (flag = false) { topLevelSetter = topLevelField = topLevelGetter; topLevelFunction(); externalTopLevelSetter = externalTopLevelGetter; diff --git a/tests/web/non_jsinterop_test.dart b/tests/web/non_jsinterop_test.dart index 550ea2755bf..6173cd94cf1 100644 --- a/tests/web/non_jsinterop_test.dart +++ b/tests/web/non_jsinterop_test.dart @@ -238,8 +238,10 @@ class JsInteropClass { external static externalStaticJsInteropMethod(); } +late bool flag; + main() { - if (false) { + if (flag = false) { topLevelSetter = topLevelField = topLevelGetter; topLevelFunction(); externalTopLevelJsInteropSetter = externalTopLevelJsInteropGetter; diff --git a/tests/web_2/jsinterop_test.dart b/tests/web_2/jsinterop_test.dart index 5ee057c16ce..bf3766c5a8e 100644 --- a/tests/web_2/jsinterop_test.dart +++ b/tests/web_2/jsinterop_test.dart @@ -241,8 +241,10 @@ class JsInteropClass { external static externalStaticJsInteropMethod(); } +bool flag; + main() { - if (false) { + if (flag = false) { topLevelSetter = topLevelField = topLevelGetter; topLevelFunction(); externalTopLevelSetter = externalTopLevelGetter; diff --git a/tests/web_2/non_jsinterop_test.dart b/tests/web_2/non_jsinterop_test.dart index 54064381db6..1e63305115b 100644 --- a/tests/web_2/non_jsinterop_test.dart +++ b/tests/web_2/non_jsinterop_test.dart @@ -240,8 +240,10 @@ class JsInteropClass { external static externalStaticJsInteropMethod(); } +bool flag; + main() { - if (false) { + if (flag = false) { topLevelSetter = topLevelField = topLevelGetter; topLevelFunction(); externalTopLevelJsInteropSetter = externalTopLevelJsInteropGetter;