VM: Fix a couple of issues in the AOT optimizer, add fast path smi multiply.

runtime/vm/aot_optimizer.cc

 // Copyright (c) 2013, 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.
 
 #include "vm/aot_optimizer.h"
 
 #include "vm/bit_vector.h"
 #include "vm/branch_optimizer.h"
 #include "vm/cha.h"
 #include "vm/compiler.h"
@@ skipping 209 matching lines @@
                                                &error_message)) {
       const intptr_t cid = Class::Handle(Z, target_function.Owner()).id();
       const ICData& ic_data = ICData::ZoneHandle(Z,
           ICData::NewFrom(*call->ic_data(), 1));
       ic_data.AddReceiverCheck(cid, target_function);
       call->set_ic_data(&ic_data);
       return true;
     }
   }
 
-  // Check if getter or setter in function's class and class is currently leaf.

[srdjan, 2016/03/22 16:37:22]

Removing this guessing will have a considerable negative performance impact on
Richard, DeltaBlue, as far as I remember (aot). Please evaluate benefit vs
performance loss before removing this optimization (it may have beed superseded
by other optimizations).

[Florian Schneider, 2016/03/22 16:51:03]

One question: Is this only meant for getter/setter invocations with "this" as
receiver or any receiver?

I'll check the numbers before landing. If there is a regression, I can also put
this back in for now. It is independent from the rest of the CL.

-  if (FLAG_guess_icdata_cid &&
-      ((call->token_kind() == Token::kGET) ||
-          (call->token_kind() == Token::kSET))) {
-    const Class& owner_class = Class::Handle(Z, function().Owner());
-    if (!owner_class.is_abstract() &&
-        !CHA::HasSubclasses(owner_class) &&
-        !CHA::IsImplemented(owner_class)) {
-      const Array& args_desc_array = Array::Handle(Z,
-          ArgumentsDescriptor::New(call->ArgumentCount(),
-                                   call->argument_names()));
-      ArgumentsDescriptor args_desc(args_desc_array);
-      const Function& function = Function::Handle(Z,
-          Resolver::ResolveDynamicForReceiverClass(owner_class,
-                                                   call->function_name(),
-                                                   args_desc));
-      if (!function.IsNull()) {
-        const ICData& ic_data = ICData::ZoneHandle(Z,
-            ICData::NewFrom(*call->ic_data(), class_ids.length()));
-        ic_data.AddReceiverCheck(owner_class.id(), function);
-        call->set_ic_data(&ic_data);
-        return true;
-      }
-    }
-  }
-
   return false;
 }
 
 
 const ICData& AotOptimizer::TrySpecializeICData(const ICData& ic_data,
                                                 intptr_t cid) {
   ASSERT(ic_data.NumArgsTested() == 1);
 
   if ((ic_data.NumberOfUsedChecks() == 1) && ic_data.HasReceiverClassId(cid)) {
     return ic_data;  // Nothing to do
@@ skipping 2132 matching lines @@
 
 
 bool AotOptimizer::IsBlackListedForInlining(intptr_t call_deopt_id) {
   for (intptr_t i = 0; i < inlining_black_list_->length(); ++i) {
     if ((*inlining_black_list_)[i] == call_deopt_id) return true;
   }
   return false;
 }
 
 
+static bool HasLikelySmiOperand(InstanceCallInstr* instr) {
+  // If one of the inputs is the result of another CheckedSmiOp, we guess
+  // the operand to be likely smi. Phis with at least one known smi are
+  // guessed to be likely smi as well.
+  for (intptr_t i = 0; i < instr->ArgumentCount(); ++i) {
+    PhiInstr* phi = instr->ArgumentAt(i)->AsPhi();
+    if (phi != NULL) {
+      for (intptr_t j = 0; j < phi->InputCount(); ++j) {
+        if (phi->InputAt(j)->Type()->ToCid() == kSmiCid) return true;
+      }
+      continue;
+    }
+    if (instr->ArgumentAt(i)->IsCheckedSmiOp()) return true;
+  }
+  return false;
+}
+
 // Tries to optimize instance call by replacing it with a faster instruction
 // (e.g, binary op, field load, ..).
 void AotOptimizer::VisitInstanceCall(InstanceCallInstr* instr) {
   ASSERT(FLAG_precompiled_mode);
   // TODO(srdjan): Investigate other attempts, as they are not allowed to
   // deoptimize.
 
   // Type test is special as it always gets converted into inlined code.
   const Token::Kind op_kind = instr->token_kind();
   if (Token::IsTypeTestOperator(op_kind)) {
@@ skipping 70 matching lines @@
   switch (instr->token_kind()) {
     case Token::kEQ:
     case Token::kLT:
     case Token::kLTE:
     case Token::kGT:
     case Token::kGTE:
     case Token::kBIT_OR:
     case Token::kBIT_XOR:
     case Token::kBIT_AND:
     case Token::kADD:
-    case Token::kSUB: {
-      if (HasOnlyTwoOf(*instr->ic_data(), kSmiCid)) {
+    case Token::kSUB:
+    case Token::kMUL: {
+      if (HasOnlyTwoOf(*instr->ic_data(), kSmiCid) ||
+          HasLikelySmiOperand(instr)) {
         Definition* left = instr->ArgumentAt(0);
         Definition* right = instr->ArgumentAt(1);
         CheckedSmiOpInstr* smi_op =
             new(Z) CheckedSmiOpInstr(instr->token_kind(),
                                      new(Z) Value(left),
                                      new(Z) Value(right),
                                      instr);
 
         ReplaceCall(instr, smi_op);
         return;
       }
+      break;
     }
     default:
       break;
   }
 
+  // No IC data checks. Try resolve target using the propagated type.
+  // If the propagated type has a method with the target name and there are
+  // no overrides with that name according to CHA, call the method directly.
+  const intptr_t receiver_cid =
+      instr->PushArgumentAt(0)->value()->Type()->ToCid();
+  if (receiver_cid != kDynamicCid) {
+    const Class& receiver_class = Class::Handle(Z,
+        isolate()->class_table()->At(receiver_cid));
+
+    const Array& args_desc_array = Array::Handle(Z,
+        ArgumentsDescriptor::New(instr->ArgumentCount(),
+                                 instr->argument_names()));
+    ArgumentsDescriptor args_desc(args_desc_array);
+    const Function& function = Function::Handle(Z,
+        Resolver::ResolveDynamicForReceiverClass(
+            receiver_class,
+            instr->function_name(),
+            args_desc));
+    if (!function.IsNull()) {
+      if (!thread()->cha()->HasOverride(receiver_class,
+                                        instr->function_name())) {
+        if (FLAG_trace_cha) {
+          THR_Print("  **(CHA) Instance call needs no check, "
+              "no overrides of '%s' '%s'\n",
+              instr->function_name().ToCString(), receiver_class.ToCString());
+        }
+        thread()->cha()->AddToLeafClasses(receiver_class);

[srdjan, 2016/03/22 16:37:22]

AddToLeafClasses is I think only needed when deoptimizing (RegisterCHACode), and
could be taken out from aot_optimizer, I believe.

[Florian Schneider, 2016/03/22 16:51:03]

Done.

+
+        // Create fake IC data with the resolved target.
+        const ICData& ic_data = ICData::Handle(
+            ICData::New(flow_graph_->function(),
+                        instr->function_name(),
+                        args_desc_array,
+                        Thread::kNoDeoptId,
+                        /* args_tested = */ 1));
+        ic_data.AddReceiverCheck(receiver_class.id(), function);
+        PolymorphicInstanceCallInstr* call =
+            new(Z) PolymorphicInstanceCallInstr(instr, ic_data,
+                                                /* with_checks = */ false);
+        instr->ReplaceWith(call, current_iterator());
+        return;
+      }
+    }
+  }
+
   // More than one targets. Generate generic polymorphic call without
   // deoptimization.
   if (instr->ic_data()->NumberOfUsedChecks() > 0) {
     ASSERT(!FLAG_polymorphic_with_deopt);
     // OK to use checks with PolymorphicInstanceCallInstr since no
     // deoptimization is allowed.
     PolymorphicInstanceCallInstr* call =
         new(Z) PolymorphicInstanceCallInstr(instr, unary_checks,
                                             /* with_checks = */ true);
     instr->ReplaceWith(call, current_iterator());
     return;
   }
-
-  // No IC data checks. Try resolve target using the propagated type.
-  // If the propagated type has a method with the target name and there are
-  // no overrides with that name according to CHA, call the method directly.
-  const intptr_t receiver_cid =
-      instr->PushArgumentAt(0)->value()->Type()->ToCid();
-  if (receiver_cid == kDynamicCid) return;
-  const Class& receiver_class = Class::Handle(Z,
-      isolate()->class_table()->At(receiver_cid));
-
-  const Array& args_desc_array = Array::Handle(Z,
-      ArgumentsDescriptor::New(instr->ArgumentCount(),
-                               instr->argument_names()));
-  ArgumentsDescriptor args_desc(args_desc_array);
-  const Function& function = Function::Handle(Z,
-      Resolver::ResolveDynamicForReceiverClass(
-          receiver_class,
-          instr->function_name(),
-          args_desc));
-  if (function.IsNull()) {
-    return;
-  }
-  if (!thread()->cha()->HasOverride(receiver_class, instr->function_name())) {
-    if (FLAG_trace_cha) {
-      THR_Print("  **(CHA) Instance call needs no check, "
-          "no overrides of '%s' '%s'\n",
-          instr->function_name().ToCString(), receiver_class.ToCString());
-    }
-    thread()->cha()->AddToLeafClasses(receiver_class);
-
-    // Create fake IC data with the resolved target.
-    const ICData& ic_data = ICData::Handle(
-        ICData::New(flow_graph_->function(),
-                    instr->function_name(),
-                    args_desc_array,
-                    Thread::kNoDeoptId,
-                    /* args_tested = */ 1));
-    ic_data.AddReceiverCheck(receiver_class.id(), function);
-    PolymorphicInstanceCallInstr* call =
-        new(Z) PolymorphicInstanceCallInstr(instr, ic_data,
-                                            /* with_checks = */ false);
-    instr->ReplaceWith(call, current_iterator());
-  }
 }
 
 
 void AotOptimizer::VisitStaticCall(StaticCallInstr* call) {
   if (!CanUnboxDouble()) {
     return;
   }
   MethodRecognizer::Kind recognized_kind =
       MethodRecognizer::RecognizeKind(call->function());
   MathUnaryInstr::MathUnaryKind unary_kind;
@@ skipping 210 matching lines @@
 
   // Discard the environment from the original instruction because the store
   // can't deoptimize.
   instr->RemoveEnvironment();
   ReplaceCall(instr, store);
   return true;
 }
 
 
 }  // namespace dart