[dart2js] `late final` initialized instance field getters are idempotent
I wanted to detect that the synthesized getter is a late final field
getter and set the 'allowCSE' property in the optimizer, but as there
is not obvious predicate, used the annotation instead.
Change-Id: Idd8797e3e7e8ececabe57e57e10c3fb9ade71477
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/430921
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Nate Biggs <natebiggs@google.com>
diff --git a/pkg/compiler/lib/src/kernel/transformations/modular/late_lowering.dart b/pkg/compiler/lib/src/kernel/transformations/modular/late_lowering.dart
index 4dd98cb..1d3ee75 100644
--- a/pkg/compiler/lib/src/kernel/transformations/modular/late_lowering.dart
+++ b/pkg/compiler/lib/src/kernel/transformations/modular/late_lowering.dart
@@ -76,6 +76,10 @@
final ExtensionTable _extensionTable = ExtensionTable();
+ late final InstanceConstant pragmaAllowCSE = _pragmaConstant(
+ 'dart2js:allow-cse',
+ );
+
LateLowering(this._coreTypes, CompilerOptions? _options)
: _omitLateNames = _options?.omitLateNames ?? false,
_readLocal = _Reader(_coreTypes.cellReadLocal),
@@ -674,6 +678,12 @@
// transformer flags to reflect whether the getter contains super calls.
getter.transformerFlags = field.transformerFlags;
_copyAnnotations(getter, field);
+ if (initializer != null && field.isFinal) {
+ getter.addAnnotation(
+ ConstantExpression(pragmaAllowCSE, _coreTypes.pragmaNonNullableRawType)
+ ..fileOffset = field.fileOffset,
+ );
+ }
enclosingClass.addProcedure(getter);
VariableDeclaration setterValue = VariableDeclaration(
@@ -750,6 +760,13 @@
}
}
+ InstanceConstant _pragmaConstant(String pragmaName) {
+ return InstanceConstant(_coreTypes.pragmaClass.reference, [], {
+ _coreTypes.pragmaName.fieldReference: StringConstant(pragmaName),
+ _coreTypes.pragmaOptions.fieldReference: NullConstant(),
+ });
+ }
+
TreeNode transformField(Field field, Member contextMember) {
_contextMember = contextMember;
diff --git a/pkg/compiler/lib/src/ssa/builder.dart b/pkg/compiler/lib/src/ssa/builder.dart
index 56ece5e..fe211a7 100644
--- a/pkg/compiler/lib/src/ssa/builder.dart
+++ b/pkg/compiler/lib/src/ssa/builder.dart
@@ -7873,11 +7873,19 @@
return false;
}
- // Don't inline functions marked with 'allow-cse' and 'allow-dce' since we
- // need the call instruction to do these optimizations. We might be able to
+ // Don't inline functions marked with 'allow-dce' since we need the call
+ // instruction to recognize the whole call as unused. We might be able to
// inline simple methods afterwards.
- if (closedWorld.annotationsData.allowCSE(function) ||
- closedWorld.annotationsData.allowDCE(function)) {
+ if (closedWorld.annotationsData.allowDCE(function)) {
+ return false;
+ }
+
+ // Don't inline functions marked with 'allow-cse' since we need the call
+ // instructions to recognize repeated calls. We might be able to inline
+ // simple methods afterwards. If this is the only call site, we will never
+ // find the repeated call, so we should consider inlining here.
+ if (closedWorld.annotationsData.allowCSE(function) &&
+ !_isCalledOnce(function)) {
return false;
}
diff --git a/pkg/compiler/lib/src/ssa/nodes.dart b/pkg/compiler/lib/src/ssa/nodes.dart
index 636be70..ee060bd 100644
--- a/pkg/compiler/lib/src/ssa/nodes.dart
+++ b/pkg/compiler/lib/src/ssa/nodes.dart
@@ -1344,6 +1344,9 @@
bool hasSameType = typeEquals(other);
assert(hasSameType == (_gvnType == other._gvnType));
if (!hasSameType) return false;
+ // Check the data first to ensure we are considering the same element or
+ // selector.
+ if (!dataEquals(other)) return false;
assert((useGvn() && other.useGvn()) || (allowCSE && other.allowCSE));
if (sideEffects != other.sideEffects) return false;
// Check that the inputs match.
@@ -1355,8 +1358,7 @@
return false;
}
}
- // Check that the data in the instruction matches.
- return dataEquals(other);
+ return true;
}
int gvnHashCode() {
@@ -2092,13 +2094,8 @@
@override
bool dataEquals(HInvokeDynamic other) {
- // Use the name and the kind instead of [Selector.operator==]
- // because we don't need to check the arity (already checked in
- // [gvnEquals]), and the receiver types may not be in sync.
- // TODO(sra): If we GVN calls with named (optional) arguments then the
- // selector needs a deeper check for the same subset of named arguments.
- return selector.name == other.selector.name &&
- selector.kind == other.selector.kind;
+ return selector == other.selector &&
+ (useGvn() == other.useGvn() || allowCSE == other.allowCSE);
}
}
diff --git a/pkg/front_end/testcases/dart2js/late_fields.dart.strong.transformed.expect b/pkg/front_end/testcases/dart2js/late_fields.dart.strong.transformed.expect
index cabb345..590ece1 100644
--- a/pkg/front_end/testcases/dart2js/late_fields.dart.strong.transformed.expect
+++ b/pkg/front_end/testcases/dart2js/late_fields.dart.strong.transformed.expect
@@ -32,6 +32,7 @@
}
set c(synthesized core::int value) → void
this.{self::C::_#C#c#AI} = value;
+ @#C3
get d() → core::int {
synthesized core::int value = this.{self::C::_#C#d#FI}{core::int};
if(_in::isSentinel(value)) {
@@ -68,6 +69,11 @@
core::print(self::c.{self::C::d}{core::int});
}
+constants {
+ #C1 = "dart2js:allow-cse"
+ #C2 = null
+ #C3 = core::pragma {name:#C1, options:#C2}
+}
Extra constant evaluation status:
Evaluated: InstanceInvocation @ org-dartlang-testcase:///late_fields.dart:15:16 -> DoubleConstant(-1.0)
diff --git a/pkg/front_end/testcases/dart2js/late_fields_with_annotation.dart.strong.transformed.expect b/pkg/front_end/testcases/dart2js/late_fields_with_annotation.dart.strong.transformed.expect
index 1228214..cb5c17d 100644
--- a/pkg/front_end/testcases/dart2js/late_fields_with_annotation.dart.strong.transformed.expect
+++ b/pkg/front_end/testcases/dart2js/late_fields_with_annotation.dart.strong.transformed.expect
@@ -42,6 +42,7 @@
this.{self::C::_#C#c#AI} = value;
@#C5
@#C9
+ @#C11
get d() → core::int {
synthesized core::int value = this.{self::C::_#C#d#FI}{core::int};
if(_in::isSentinel(value)) {
@@ -88,6 +89,8 @@
#C7 = core::pragma {name:#C6, options:#C2}
#C8 = "dart2js:noInline"
#C9 = core::pragma {name:#C8, options:#C2}
+ #C10 = "dart2js:allow-cse"
+ #C11 = core::pragma {name:#C10, options:#C2}
}
Extra constant evaluation status:
diff --git a/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.expect b/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.expect
index 2d935e1..053ab4b 100644
--- a/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.expect
+++ b/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.expect
@@ -119,6 +119,7 @@
}
set c(synthesized core::int value) → void
this.{mai::C::_#C#c#AI} = value;
+ @#C3
get d() → core::int {
synthesized core::int value = this.{mai::C::_#C#d#FI}{core::int};
if(_in::isSentinel(value)) {
@@ -212,3 +213,9 @@
return mai2::_#b.{_la::_Cell::readField}<core::int>(){() → core::int};
static set b(synthesized core::int value) → void
return mai2::_#b.{_la::_Cell::finalFieldValue} = value;
+
+constants {
+ #C1 = "dart2js:allow-cse"
+ #C2 = null
+ #C3 = core::pragma {name:#C1, options:#C2}
+}
diff --git a/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.transformed.expect b/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.transformed.expect
index 4128801..0d24073 100644
--- a/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.transformed.expect
+++ b/pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.transformed.expect
@@ -119,6 +119,7 @@
}
set c(synthesized core::int value) → void
this.{mai::C::_#C#c#AI} = value;
+ @#C3
get d() → core::int {
synthesized core::int value = this.{mai::C::_#C#d#FI}{core::int};
if(_in::isSentinel(value)) {
@@ -213,6 +214,11 @@
static set b(synthesized core::int value) → void
return mai2::_#b.{_la::_Cell::finalFieldValue} = value;
+constants {
+ #C1 = "dart2js:allow-cse"
+ #C2 = null
+ #C3 = core::pragma {name:#C1, options:#C2}
+}
Extra constant evaluation status:
Evaluated: InstanceInvocation @ org-dartlang-testcase:///main_lib1.dart:8:16 -> DoubleConstant(-1.0)
