Throw NoSuchMethod or ArgumentError instead of generating a bailout, when we know the next instruct…

sdk/lib/_internal/compiler/implementation/ssa/bailout.dart

Index: sdk/lib/_internal/compiler/implementation/ssa/bailout.dart
===================================================================
--- sdk/lib/_internal/compiler/implementation/ssa/bailout.dart	(revision 21808)
+++ sdk/lib/_internal/compiler/implementation/ssa/bailout.dart	(working copy)
@@ -67,6 +67,7 @@
   final CodegenWorkItem work;
   bool calledInLoop = false;
   bool isRecursiveMethod = false;
+  bool hasInsertedChecks = false;
   int stateId = 1;
   Map<HInstruction, HType> savedTypes = new Map<HInstruction, HType>();
 
@@ -190,6 +191,30 @@
     return calledInLoop;
   }
 
+  // Returns whether an invcation of [selector] on [receiver] will throw a

[kasperl, 2013/04/23 06:12:43]

invcation -> invocation

[ngeoffray, 2013/04/23 08:06:04]

Done.

+  // [ArgumentError] if the argument is not of the right type.
+  bool willThrowArgumentError(Selector selector, HInstruction receiver) {
+    if (receiver != null && (receiver.isInteger() || receiver.isString())) {
+      return selector.isOperator() && selector.name != const SourceString('==');
+    }
+    return false;
+  }
+
+  // Returns whether an invocation of [selector] will throw a
+  // [NoSuchMethodError] if the receiver is not of the type
+  // [speculativeType].
+  bool willThrowNoSuchMethodErrorIfNot(Selector selector,
+                                       HType speculativeType) {
+    return compiler.world.hasSingleMatch(selector)
+        // In some cases, we want the receiver to be an integer,
+        // but that does not mean we will get a NoSuchMethodError
+        // if it's not: the receiver could be a double.
+        && !speculativeType.isInteger()
+        // We speculate on the [operator==] instruction, but we know it
+        // will never throw a [NoSuchMethodError].
+        && selector.name != const SourceString('==');
+  }
+
   bool shouldInsertTypeGuard(HInstruction instruction, HType speculativeType) {
     if (!speculativeType.isUseful()) return false;
     // If the types agree we don't need to check.
@@ -218,28 +243,98 @@
       } else {
         insertionPoint = instruction.next;
       }
-      // If the previous instruction is also a type guard, then both
-      // guards have the same environment, and can therefore share the
-      // same state id.
-      HBailoutTarget target;
-      int state;
-      if (insertionPoint.previous is HTypeGuard) {
-        HTypeGuard other = insertionPoint.previous;
-        target = other.bailoutTarget;
+
+      // Find out if we should actually just emit a check for it. If

[kasperl, 2013/04/23 06:12:43]

It would be nice if this could be refactored into a bunch of nicer helper
methods with good names.

[ngeoffray, 2013/04/23 08:06:04]

Done.

+      // there is a user of [instruction] in the same block (so that
+      // we know it will be executed), and that user has a selector
+      // (meaning it will be a call), we can put a type check instead
+      // of a type guard, that will either throw a [NoSuchMethodError]
+      // or a [ArgumentError].
+      bool willThrow = false;
+      Selector receiverSelectorOnThrow = null;
+      HInstruction firstUserWithSelector;
+      HInstruction firstUser;
+      // For a parameter, we look at the first block that contains
+      // user instructions.
+      HBasicBlock userMustBeInBlock = instruction is HParameterValue
+          ? instruction.block.successors[0]
+          : instruction.block;
+
+      // We distinguish between [firstUser] and
+      // [firstUserWithSelector] because we can often have the pattern
+      // that [firstUser] is a call to [: getInterceptor :] and
+      // [firstUserWithSelector] is the actual user of the
+      // instruction.
+      for (HInstruction user in instruction.usedBy) {

[kasperl, 2013/04/23 06:12:43]

Somehow it feels like we could use a better strategy here. If we inserted
throwing type guards in front of all throwing users we wouldn't need the
same-block restriction. We could always work on moving the throwing guards up
the dominator tree and eliminating redundant ones later (maybe that could be
done in general for type guards so we wouldn't have to insert them so early?).

Wouldn't that also eliminate the rather weird distinction between firstUser and
firstUserWithSelector?

[ngeoffray, 2013/04/23 08:06:04]

Agree. After discussing a bit about it, I'll work on another CL that will put
type guards at relevant use site, and moving them up when needed.
Actually, I don't need this distinction, I just changed the code to update all
users after the first user that has a selector.

+        if (user.block == userMustBeInBlock) {
+          if (firstUser == null || user.dominates(firstUser)) {
+            firstUser = user;
+            if (user.selector != null) {
+              firstUserWithSelector = user;
+            }
+          } else if (user.selector != null) {
+            if (firstUserWithSelector == null
+                || user.dominates(firstUserWithSelector)) {
+              firstUserWithSelector = user;
+            }
+          }
+        }
+      }
+
+      // If we have found a user with a selector, we find out if it
+      // will throw [NoSuchMethodError] or [ArgumentError].
+      if (firstUserWithSelector != null
+          && (firstUserWithSelector == firstUser
+              || firstUser.next == firstUserWithSelector)) {
+        assert(firstUser == firstUserWithSelector || !firstUser.hasSideEffects());

[kasperl, 2013/04/23 06:12:43]

Long line.

[ngeoffray, 2013/04/23 08:06:04]

Done.

+        Selector selector = firstUserWithSelector.selector;
+        HInstruction receiver = firstUserWithSelector.getDartReceiver(compiler);
+        if (receiver == instruction) {
+          if (willThrowNoSuchMethodErrorIfNot(selector, speculativeType)) {
+            receiverSelectorOnThrow = firstUserWithSelector.selector;
+            willThrow = true;
+          }
+        } else if (willThrowArgumentError(selector, receiver)) {
+          willThrow = true;
+        }
+      }
+
+      var check;
+      if (willThrow) {
+        insertionPoint = firstUser;
+        check = new HTypeConversion(
+            null,
+            receiverSelectorOnThrow == null
+                ? HTypeConversion.ARGUMENT_TYPE_CHECK
+                : HTypeConversion.RECEIVER_TYPE_CHECK,
+            speculativeType,
+            instruction,
+            receiverSelectorOnThrow);
+        hasInsertedChecks = true;
       } else {
-        state = stateId++;
-        target = new HBailoutTarget(state);
-        insertionPoint.block.addBefore(insertionPoint, target);
+        // If the previous instruction is also a type guard, then both
+        // guards have the same environment, and can therefore share the
+        // same state id.
+        HBailoutTarget target;
+        int state;
+        if (insertionPoint.previous is HTypeGuard) {
+          HTypeGuard other = insertionPoint.previous;
+          target = other.bailoutTarget;
+        } else {
+          state = stateId++;
+          target = new HBailoutTarget(state);
+          insertionPoint.block.addBefore(insertionPoint, target);
+        }
+        check = new HTypeGuard(speculativeType, instruction, target);
+        work.guards.add(check);
+        // By setting the type of the guard to the speculated type, we
+        // help the analysis find valuable type guards. This however
+        // requires to run a non-speculative type propagation again
+        // after this analysis.
+        check.instructionType = speculativeType;
       }
-      HTypeGuard guard = new HTypeGuard(speculativeType, instruction, target);
-      work.guards.add(guard);
-      // By setting the type of the guard to the speculated type, we
-      // help the analysis find valuable type guards. This however
-      // requires to run a non-speculative type propagation again
-      // after this analysis.
-      guard.instructionType = speculativeType;
-      instruction.block.rewrite(instruction, guard);
-      insertionPoint.block.addBefore(insertionPoint, guard);
+      instruction.block.rewrite(instruction, check);
+      insertionPoint.block.addBefore(insertionPoint, check);
     }
     return hasChanged;
   }