[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 <[email protected]>
Commit-Queue: Stephen Adams <[email protected]>
Reviewed-by: Nate Biggs <[email protected]>

Author: rakudrama

pkg/compiler/lib/src/kernel/transformations/modular/late_lowering.dart

@@ -76,6 +76,10 @@ class LateLowering {
 
   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 @@ class LateLowering {
     // 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 @@ class LateLowering {
     }
   }
 
+  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;
 

pkg/compiler/lib/src/ssa/builder.dart

@@ -7873,11 +7873,19 @@ class KernelSsaGraphBuilder extends ir.VisitorDefault<void>
       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;
     }
 

pkg/compiler/lib/src/ssa/nodes.dart

@@ -1344,6 +1344,9 @@ abstract class HInstruction implements SpannableWithEntity {
     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 @@ abstract class HInstruction implements SpannableWithEntity {
         return false;
       }
     }
-    // Check that the data in the instruction matches.
-    return dataEquals(other);
+    return true;
   }
 
   int gvnHashCode() {
@@ -2092,13 +2094,8 @@ abstract class HInvokeDynamic extends HInvoke implements InstructionContext {
 
   @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);
   }
 }
 

pkg/front_end/testcases/dart2js/late_fields.dart.strong.transformed.expect

@@ -32,6 +32,7 @@ class C extends core::Object {
   }
   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 @@ static method testInitializedFinalInstanceField() → void {
   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)

pkg/front_end/testcases/dart2js/late_fields_with_annotation.dart.strong.transformed.expect

@@ -42,6 +42,7 @@ class C extends core::Object {
     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 @@ constants  {
   #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:

pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.expect

@@ -119,6 +119,7 @@ class C extends core::Object {
   }
   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 @@ static get b() → core::int
   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}
+}

pkg/front_end/testcases/dart2js/late_from_dill/main.dart.strong.transformed.expect

@@ -119,6 +119,7 @@ class C extends core::Object {
   }
   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 get b() → 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}
+}
 
 Extra constant evaluation status:
 Evaluated: InstanceInvocation @ org-dartlang-testcase:///main_lib1.dart:8:16 -> DoubleConstant(-1.0)