Revert "[dart2js] Avoid premature interceptor optimizations"
This reverts commit db6f65e661a8d44f57c5c03141064f58d819ea86.
Reason for revert: b/184605569
Original change's description:
> [dart2js] Avoid premature interceptor optimizations
>
> Add a SsaFinalizeInterceptors phase that does optimizations with the
> interceptor calling convention.
>
> This puts the the caller and callee sides of the dummy explicit
> receiver optimization in the same phase.
>
> Change-Id: I07fe0483c2658c3744b76a235edb24720d7ea60f
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193140
> Commit-Queue: Stephen Adams <sra@google.com>
> Reviewed-by: Mayank Patke <fishythefish@google.com>
# Not skipping CQ checks because original CL landed > 1 day ago.
Change-Id: I0e2959f329030f12a43a9aef16cfee9b94bbf250
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194061
Reviewed-by: Ivan Inozemtsev <iinozemtsev@google.com>
Commit-Queue: Ivan Inozemtsev <iinozemtsev@google.com>
diff --git a/pkg/compiler/lib/src/ssa/codegen.dart b/pkg/compiler/lib/src/ssa/codegen.dart
index f6928ab..81f1761 100644
--- a/pkg/compiler/lib/src/ssa/codegen.dart
+++ b/pkg/compiler/lib/src/ssa/codegen.dart
@@ -414,7 +414,8 @@
assert(graph.isValid(), 'Graph not valid after ${phase.name}');
}
- runPhase(new SsaInstructionSelection(_options, _closedWorld));
+ runPhase(
+ new SsaInstructionSelection(_options, _closedWorld, _interceptorData));
runPhase(new SsaTypeKnownRemover());
runPhase(new SsaTrustedCheckRemover(_options));
runPhase(new SsaAssignmentChaining(_closedWorld));
diff --git a/pkg/compiler/lib/src/ssa/codegen_helpers.dart b/pkg/compiler/lib/src/ssa/codegen_helpers.dart
index 5bf162b..d643936 100644
--- a/pkg/compiler/lib/src/ssa/codegen_helpers.dart
+++ b/pkg/compiler/lib/src/ssa/codegen_helpers.dart
@@ -5,6 +5,7 @@
import '../constants/values.dart';
import '../elements/entities.dart';
import '../inferrer/abstract_value_domain.dart';
+import '../js_backend/interceptor_data.dart';
import '../options.dart';
import '../universe/selector.dart' show Selector;
import '../world.dart' show JClosedWorld;
@@ -26,14 +27,21 @@
///
/// - Remove NullChecks where the next instruction would fail on the operand.
///
+/// - Dummy receiver optimization.
+///
+/// - One-shot interceptor optimization.
+///
/// - Combine read/modify/write sequences into HReadModifyWrite instructions to
/// simplify codegen of expressions like `a.x += y`.
+
class SsaInstructionSelection extends HBaseVisitor with CodegenPhase {
final JClosedWorld _closedWorld;
+ final InterceptorData _interceptorData;
final CompilerOptions _options;
HGraph graph;
- SsaInstructionSelection(this._options, this._closedWorld);
+ SsaInstructionSelection(
+ this._options, this._closedWorld, this._interceptorData);
AbstractValueDomain get _abstractValueDomain =>
_closedWorld.abstractValueDomain;
@@ -221,6 +229,128 @@
.isPotentiallyTrue;
@override
+ HInstruction visitInvokeDynamic(HInvokeDynamic node) {
+ if (!node.isInterceptedCall) return node;
+
+ tryReplaceExplicitReceiverWithDummy(
+ node, node.selector, node.element, node.receiverType);
+
+ // Try to replace
+ //
+ // getInterceptor(o).method(o, ...)
+ //
+ // with a 'one shot interceptor' which is a call to a synthesized static
+ // helper function that combines the two operations.
+ //
+ // oneShotMethod(o, 1, 2)
+ //
+ // This saves code size and makes the receiver of an intercepted call a
+ // candidate for being generated at use site.
+ //
+ // Avoid combining a hoisted interceptor back into a loop, and the faster
+ // almost-constant kind of interceptor.
+
+ HInstruction interceptor = node.inputs[0];
+ if (interceptor is HInterceptor &&
+ interceptor.usedBy.length == 1 &&
+ !interceptor.isConditionalConstantInterceptor &&
+ interceptor.hasSameLoopHeaderAs(node)) {
+ // Copy inputs and replace interceptor with `null`.
+ List<HInstruction> inputs = List.of(node.inputs);
+ inputs[0] = graph.addConstantNull(_closedWorld);
+
+ HOneShotInterceptor oneShot = HOneShotInterceptor(
+ node.selector,
+ node.receiverType,
+ inputs,
+ node.instructionType,
+ node.typeArguments,
+ interceptor.interceptedClasses);
+ oneShot.sourceInformation = node.sourceInformation;
+ oneShot.sourceElement = node.sourceElement;
+ oneShot.sideEffects.setTo(node.sideEffects);
+
+ HBasicBlock block = node.block;
+ block.addAfter(node, oneShot);
+ block.rewrite(node, oneShot);
+ block.remove(node);
+ interceptor.block.remove(interceptor);
+ return null;
+ }
+
+ return node;
+ }
+
+ @override
+ HInstruction visitInvokeSuper(HInvokeSuper node) {
+ tryReplaceExplicitReceiverWithDummy(
+ node, node.selector, node.element, null);
+ return node;
+ }
+
+ @override
+ HInstruction visitOneShotInterceptor(HOneShotInterceptor node) {
+ throw StateError('Should not see HOneShotInterceptor: $node');
+ }
+
+ void tryReplaceExplicitReceiverWithDummy(HInvoke node, Selector selector,
+ MemberEntity target, AbstractValue mask) {
+ // Calls of the form
+ //
+ // a.foo$1(a, x)
+ //
+ // where the interceptor calling convention is used come from recognizing
+ // that 'a' is a 'self-interceptor'. If the selector matches only methods
+ // that ignore the explicit receiver parameter, replace occurrences of the
+ // receiver argument with a dummy receiver '0':
+ //
+ // a.foo$1(a, x) ---> a.foo$1(0, x)
+ //
+ // This often reduces the number of references to 'a' to one, allowing 'a'
+ // to be generated at use to avoid a temporary, e.g.
+ //
+ // t1 = b.get$thing();
+ // t1.foo$1(t1, x)
+ // --->
+ // b.get$thing().foo$1(0, x)
+ //
+ assert(target != null || mask != null);
+
+ if (!node.isInterceptedCall) return;
+
+ // TODO(15933): Make automatically generated property extraction closures
+ // work with the dummy receiver optimization.
+ if (selector.isGetter) return;
+
+ // This assignment of inputs is uniform for HInvokeDynamic and HInvokeSuper.
+ HInstruction interceptor = node.inputs[0];
+ HInstruction receiverArgument = node.inputs[1];
+
+ // A 'self-interceptor'?
+ if (interceptor.nonCheck() != receiverArgument.nonCheck()) return;
+
+ // TODO(sra): Should this be an assert?
+ if (!_interceptorData.isInterceptedSelector(selector)) return;
+
+ if (target != null) {
+ // A call that resolves to a single instance method (element) requires the
+ // calling convention consistent with the method.
+ ClassEntity cls = target.enclosingClass;
+ assert(_interceptorData.isInterceptedMethod(target));
+ if (_interceptorData.isInterceptedClass(cls)) return;
+ } else if (_interceptorData.isInterceptedMixinSelector(
+ selector, mask, _closedWorld)) {
+ return;
+ }
+
+ ConstantValue constant = DummyInterceptorConstantValue();
+ HConstant dummy = graph.addConstant(constant, _closedWorld);
+ receiverArgument.usedBy.remove(node);
+ node.inputs[1] = dummy;
+ dummy.usedBy.add(node);
+ }
+
+ @override
HInstruction visitFieldSet(HFieldSet setter) {
// Pattern match
// t1 = x.f; t2 = t1 + 1; x.f = t2; use(t2) --> ++x.f
diff --git a/pkg/compiler/lib/src/ssa/interceptor_finalizer.dart b/pkg/compiler/lib/src/ssa/interceptor_finalizer.dart
deleted file mode 100644
index 9ec829d..0000000
--- a/pkg/compiler/lib/src/ssa/interceptor_finalizer.dart
+++ /dev/null
@@ -1,240 +0,0 @@
-// Copyright (c) 2021, 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 '../constants/values.dart';
-import '../elements/entities.dart';
-import '../inferrer/abstract_value_domain.dart';
-import '../js_backend/interceptor_data.dart';
-import '../universe/selector.dart' show Selector;
-import '../world.dart' show JClosedWorld;
-import 'nodes.dart';
-import 'optimize.dart';
-
-/// SsaFinalizeInterceptors makes adjustments for the interceptor calling
-/// convention.
-///
-/// 1. If the method cannot be invoked with an intercepted receiver, the
-/// receiver and interceptor are the same. In this case ignore the explicit
-/// receiver argument and use the interceptor (this) as the receiver.
-///
-/// 2. The call-site dual of the above is if a method ignores the explicit
-/// receiver, it can be replaced with a dummy value, i.e. a dummy receiver
-/// optimization.
-///
-/// 3. If an interceptor is used once for a call, replace the
-/// getInterceptor-call pair with a call to a 'one-shot interceptor' outlined
-/// method.
-///
-class SsaFinalizeInterceptors extends HBaseVisitor
- implements OptimizationPhase {
- @override
- String get name => "SsaFinalizeInterceptors";
- final JClosedWorld _closedWorld;
- HGraph _graph;
-
- SsaFinalizeInterceptors(this._closedWorld);
-
- InterceptorData get _interceptorData => _closedWorld.interceptorData;
-
- @override
- void visitGraph(HGraph graph) {
- _graph = graph;
- MemberEntity element = graph.element;
-
- if (usesSelfInterceptor(element)) {
- _redirectReceiver();
- }
- visitDominatorTree(graph);
- }
-
- @override
- visitBasicBlock(HBasicBlock node) {
- HInstruction instruction = node.first;
- while (instruction != null) {
- final next = instruction.next;
- instruction.accept(this);
- instruction = next;
- }
- }
-
- /// Returns `true` if [element] is an instance method that uses the
- /// interceptor calling convention but the instance and interceptor arguments
- /// will always be the same value.
- bool usesSelfInterceptor(MemberEntity element) {
- if (!_interceptorData.isInterceptedMethod(element)) return false;
- ClassEntity cls = element.enclosingClass;
- return !_interceptorData.isInterceptedClass(cls);
- }
-
- void _redirectReceiver() {
- // The entry block contains the parameters in order, starting with `this`,
- // and then the explicit receiver. There are other instructions in the
- // block, like constants, which we ignore.
- HThis thisParameter;
- HParameterValue receiverParameter;
- for (HInstruction node = _graph.entry.first;
- node != null;
- node = node.next) {
- if (node is HParameterValue) {
- if (node is HThis) {
- thisParameter = node;
- } else {
- receiverParameter = node;
- break;
- }
- }
- }
- assert(thisParameter != null,
- '`this` parameter should be before other parameters');
- assert(receiverParameter != null,
- 'Intercepted convention requires explicit receiver');
- thisParameter.instructionType = receiverParameter.instructionType;
- receiverParameter.block.rewrite(receiverParameter, thisParameter);
- receiverParameter.sourceElement = const _RenameToUnderscore();
- }
-
- @override
- void visitInvokeDynamic(HInvokeDynamic node) {
- if (!node.isInterceptedCall) return;
-
- if (node.element != null) {
- tryReplaceExplicitReceiverForTargetWithDummy(
- node, node.selector, node.element);
- } else {
- tryReplaceExplicitReceiverForSelectorWithDummy(
- node, node.selector, node.receiverType);
- }
-
- // Try to replace
- //
- // getInterceptor(o).method(o, ...)
- //
- // with a 'one shot interceptor' which is a call to a synthesized static
- // helper function that combines the two operations.
- //
- // oneShotMethod(o, 1, 2)
- //
- // This saves code size and makes the receiver of an intercepted call a
- // candidate for being generated at use site.
- //
- // Avoid combining a hoisted interceptor back into a loop, and the faster
- // almost-constant kind of interceptor.
-
- HInstruction interceptor = node.inputs[0];
- if (interceptor is HInterceptor &&
- interceptor.usedBy.length == 1 &&
- !interceptor.isConditionalConstantInterceptor &&
- interceptor.hasSameLoopHeaderAs(node)) {
- // Copy inputs and replace interceptor with `null`.
- List<HInstruction> inputs = List.of(node.inputs);
- inputs[0] = _graph.addConstantNull(_closedWorld);
-
- HOneShotInterceptor oneShot = HOneShotInterceptor(
- node.selector,
- node.receiverType,
- inputs,
- node.instructionType,
- node.typeArguments,
- interceptor.interceptedClasses);
- oneShot.sourceInformation = node.sourceInformation;
- oneShot.sourceElement = node.sourceElement;
- oneShot.sideEffects.setTo(node.sideEffects);
-
- HBasicBlock block = node.block;
- block.addAfter(node, oneShot);
- block.rewrite(node, oneShot);
- block.remove(node);
- interceptor.block.remove(interceptor);
- }
- }
-
- @override
- void visitInvokeSuper(HInvokeSuper node) {
- if (!node.isInterceptedCall) return;
- tryReplaceExplicitReceiverForTargetWithDummy(
- node, node.selector, node.element);
- }
-
- @override
- void visitInvokeGeneratorBody(HInvokeGeneratorBody node) {
- tryReplaceExplicitReceiverForTargetWithDummy(node, null, node.element);
- }
-
- @override
- void visitOneShotInterceptor(HOneShotInterceptor node) {
- throw StateError('Should not see HOneShotInterceptor: $node');
- }
-
- void tryReplaceExplicitReceiverForTargetWithDummy(
- HInvoke node, Selector selector, MemberEntity target) {
- assert(target != null);
-
- // TODO(15933): Make automatically generated property extraction closures
- // work with the dummy receiver optimization.
- if (selector != null && selector.isGetter) return;
-
- if (usesSelfInterceptor(target)) {
- _replaceReceiverArgumentWithDummy(node, 1);
- }
- }
-
- void tryReplaceExplicitReceiverForSelectorWithDummy(
- HInvoke node, Selector selector, AbstractValue mask) {
- assert(mask != null);
- // Calls of the form
- //
- // a.foo$1(a, x)
- //
- // where the interceptor calling convention is used come from recognizing
- // that 'a' is a 'self-interceptor'. If the selector matches only methods
- // that ignore the explicit receiver parameter, replace occurrences of the
- // receiver argument with a dummy receiver '0':
- //
- // a.foo$1(a, x) ---> a.foo$1(0, x)
- //
- // This often reduces the number of references to 'a' to one, allowing 'a'
- // to be generated at use to avoid a temporary, e.g.
- //
- // t1 = b.get$thing();
- // t1.foo$1(t1, x)
- // --->
- // b.get$thing().foo$1(0, x)
- //
-
- // TODO(15933): Make automatically generated property extraction closures
- // work with the dummy receiver optimization.
- if (selector.isGetter) return;
-
- // This assignment of inputs is uniform for HInvokeDynamic and HInvokeSuper.
- HInstruction interceptor = node.inputs[0];
- HInstruction receiverArgument = node.inputs[1];
-
- // A 'self-interceptor'?
- if (interceptor.nonCheck() != receiverArgument.nonCheck()) return;
-
- // TODO(sra): Should this be an assert?
- if (!_interceptorData.isInterceptedSelector(selector)) return;
-
- if (!_interceptorData.isInterceptedMixinSelector(
- selector, mask, _closedWorld)) {
- _replaceReceiverArgumentWithDummy(node, 1);
- }
- }
-
- void _replaceReceiverArgumentWithDummy(HInvoke node, int receiverIndex) {
- HInstruction receiverArgument = node.inputs[receiverIndex];
- ConstantValue constant = DummyInterceptorConstantValue();
- HConstant dummy = _graph.addConstant(constant, _closedWorld);
- receiverArgument.usedBy.remove(node);
- node.inputs[receiverIndex] = dummy;
- dummy.usedBy.add(node);
- }
-}
-
-/// A simple Entity to rename the unused receiver to `_` in non-minified code.
-class _RenameToUnderscore implements Entity {
- const _RenameToUnderscore();
- @override
- String get name => '_';
-}
diff --git a/pkg/compiler/lib/src/ssa/interceptor_simplifier.dart b/pkg/compiler/lib/src/ssa/interceptor_simplifier.dart
index c0b6254..968cf4f 100644
--- a/pkg/compiler/lib/src/ssa/interceptor_simplifier.dart
+++ b/pkg/compiler/lib/src/ssa/interceptor_simplifier.dart
@@ -12,16 +12,20 @@
import 'nodes.dart';
import 'optimize.dart';
-/// This phase computes the set of classes dispatched by an interceptor, and
-/// simplifies interceptors in multiple ways:
+/// This phase simplifies interceptors in multiple ways:
///
/// 1) If the interceptor is for an object whose type is known, it
-/// tries to use a constant interceptor instead.
+/// tries to use a constant interceptor instead.
///
/// 2) Interceptors are specialized based on the selector it is used with.
///
/// 3) If we know the object is not intercepted, we just use the object
-/// instead.
+/// instead.
+///
+/// 4) Single use interceptors at dynamic invoke sites are replaced with 'one
+/// shot interceptors' which are synthesized static helper functions that fetch
+/// the interceptor and then call the method. This saves code size and makes the
+/// receiver of an intercepted call a candidate for being generated at use site.
///
class SsaSimplifyInterceptors extends HBaseVisitor
implements OptimizationPhase {
diff --git a/pkg/compiler/lib/src/ssa/locals_handler.dart b/pkg/compiler/lib/src/ssa/locals_handler.dart
index 502d335..ea01915 100644
--- a/pkg/compiler/lib/src/ssa/locals_handler.dart
+++ b/pkg/compiler/lib/src/ssa/locals_handler.dart
@@ -288,7 +288,9 @@
element.isGenerativeConstructor &&
_nativeData.isNativeOrExtendsNative(cls);
if (_interceptorData.isInterceptedMethod(element)) {
- SyntheticLocal parameter = createLocal('receiver');
+ bool isInterceptedClass = _interceptorData.isInterceptedClass(cls);
+ String name = isInterceptedClass ? 'receiver' : '_';
+ SyntheticLocal parameter = createLocal(name);
HParameterValue value = new HParameterValue(parameter, getTypeOfThis());
builder.graph.explicitReceiverParameter = value;
builder.graph.entry.addAfter(directLocals[scopeInfo.thisLocal], value);
@@ -296,7 +298,10 @@
// If this is the first parameter inserted, make sure it stays first.
builder.lastAddedParameter = value;
}
- directLocals[scopeInfo.thisLocal] = value;
+ if (isInterceptedClass) {
+ // Only use the extra parameter in intercepted classes.
+ directLocals[scopeInfo.thisLocal] = value;
+ }
} else if (isNativeUpgradeFactory) {
SyntheticLocal parameter = createLocal('receiver');
// Unlike `this`, receiver is nullable since direct calls to generative
diff --git a/pkg/compiler/lib/src/ssa/optimize.dart b/pkg/compiler/lib/src/ssa/optimize.dart
index 8a819a1..e0f7369 100644
--- a/pkg/compiler/lib/src/ssa/optimize.dart
+++ b/pkg/compiler/lib/src/ssa/optimize.dart
@@ -34,7 +34,6 @@
import '../util/util.dart';
import '../world.dart' show JClosedWorld;
import 'interceptor_simplifier.dart';
-import 'interceptor_finalizer.dart';
import 'logging.dart';
import 'nodes.dart';
import 'types.dart';
@@ -130,9 +129,9 @@
];
phases.forEach(runPhase);
- // Simplifying interceptors is just an optimization, it is required for
- // implementation correctness because the code generator assumes it is
- // always performed to compute the intercepted classes sets.
+ // Simplifying interceptors is not strictly just an optimization, it is
+ // required for implementation correctness because the code generator
+ // assumes it is always performed.
runPhase(new SsaSimplifyInterceptors(closedWorld, member.enclosingClass));
SsaDeadCodeEliminator dce = new SsaDeadCodeEliminator(closedWorld, this);
@@ -164,13 +163,6 @@
}
phases.forEach(runPhase);
});
-
- // SsaFinalizeInterceptors must always be run to ensure consistent calling
- // conventions between SSA-generated code and other code fragments generated
- // by the emitter.
- // TODO(sra): Generate these other fragments via SSA, then this phase
- // becomes an opt-in optimization.
- runPhase(SsaFinalizeInterceptors(closedWorld));
}
}
