Optimize 2/3 of instance field increment operation.

vm/opt_code_generator_ia32.cc

Index: vm/opt_code_generator_ia32.cc
===================================================================
--- vm/opt_code_generator_ia32.cc	(revision 1054)
+++ vm/opt_code_generator_ia32.cc	(working copy)
@@ -555,16 +555,22 @@
 }
 
 
-static bool NodeHasOnlyClass(AstNode* node, const Class& cls) {
+static bool AtIdNodeHasOnlyClass(AstNode* node, intptr_t id, const Class& cls) {
   ASSERT(node != NULL);
   ASSERT(!cls.IsNull());
-  const ZoneGrowableArray<const Class*>* classes = CollectedClassesAtNode(node);
-  return (classes != NULL) &&
-         (classes->length() == 1) &&
-         ((*classes)[0]->raw() == cls.raw());
+  const ICData& ic_data = node->ICDataAtId(id);
+  if (ic_data.NumberOfArgumentsChecked() != 1 ||
+      ic_data.NumberOfChecks() != 1) {

[regis, 2011/11/02 02:02:54]

Missing 2 pairs of parenthesis.

[srdjan, 2011/11/02 15:26:20]

Done.

+    return false;
+  }
+  Class& target_cls = Class::Handle();
+  Function& target = Function::Handle();
+  ic_data.GetOneClassCheckAt(0, &target_cls, &target);
+  return target_cls.raw() == cls.raw();
 }
 
 
+
 // Implement with slow case so that it can work both with Smi and Mint types.
 void OptimizingCodeGenerator::GenerateSmiShiftBinaryOp(BinaryOpNode* node) {
   ASSERT(node->kind() == Token::kSHL);
@@ -1029,17 +1035,19 @@
   }
 
   ObjectStore* object_store = Isolate::Current()->object_store();
-  if (NodeHasOnlyClass(node, smi_class_)) {
+  if (AtIdNodeHasOnlyClass(node, node->id(), smi_class_)) {
     GenerateSmiBinaryOp(node);
     return;
   }
 
-  if (NodeHasOnlyClass(node, double_class_)) {
+  if (AtIdNodeHasOnlyClass(node, node->id(), double_class_)) {
     GenerateDoubleBinaryOp(node);
     return;
   }
 
-  if (NodeHasOnlyClass(node, Class::Handle(object_store->mint_class()))) {
+  if (AtIdNodeHasOnlyClass(node,
+                           node->id(),
+                           Class::Handle(object_store->mint_class()))) {
     GenerateMintBinaryOp(node, false);
     return;
   }
@@ -1065,7 +1073,7 @@
   }
   const char* kOptMessage = "Inlines IncrOpLocal";
   ASSERT((node->kind() == Token::kINCR) || (node->kind() == Token::kDECR));
-  if (!NodeHasOnlyClass(node, smi_class_)) {
+  if (!AtIdNodeHasOnlyClass(node, node->id(), smi_class_)) {
     TraceNotOpt(node, kOptMessage);
     CodeGenerator::VisitIncrOpLocalNode(node);
     return;
@@ -1099,6 +1107,77 @@
 }
 
 
+void OptimizingCodeGenerator::VisitIncrOpInstanceFieldNode(
+    IncrOpInstanceFieldNode* node) {
+  ASSERT((node->kind() == Token::kINCR) || (node->kind() == Token::kDECR));
+  VisitLoadOne(node->receiver(), EBX);
+  __ pushl(EBX);  // Duplicate receiver (preserve for setter).
+  InlineInstanceGetter(node,
+                       node->getter_id(),
+                       node->receiver(),
+                       node->field_name(),
+                       EBX);
+  // result is in EAX.
+  __ popl(EDX);   // Get receiver.
+  const bool has_pre_inc_result = !node->prefix() && IsResultNeeded(node);

[regis, 2011/11/02 02:02:54]

The name of the flag is a bit confusing, "has_pre" indicates "not a prefix
operator".

[srdjan, 2011/11/02 15:26:20]

Changed to : return_original_value

+  const Immediate one_value = Immediate(reinterpret_cast<int32_t>(Smi::New(1)));

[regis, 2011/11/02 02:02:54]

You can hide the cast with Immediate(Smi::RawValue(1))

[srdjan, 2011/11/02 15:26:20]

Done.

+  // EAX: Value.
+  // EDX: Receiver.
+  if (AtIdNodeHasOnlyClass(node, node->operator_id(), smi_class_)) {
+    // Deoptimization point for this node is after receiver has been
+    // pushed twice on stack and before the getter (above) was executed.
+    DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EDX, EDX);
+    if (has_pre_inc_result) {
+      // Preserve pre increment result.
+      __ movl(ECX, EAX);
+    }
+    __ testl(EAX, Immediate(kSmiTagMask));
+    __ j(NOT_ZERO, deopt_blob->label());
+    if (node->kind() == Token::kINCR) {
+      __ addl(EAX, one_value);
+    } else {
+      __ subl(EAX, one_value);
+    }
+    __ j(OVERFLOW, deopt_blob->label());
+    if (has_pre_inc_result) {
+      // Preserve as result.
+      __ pushl(ECX);  // Preserve pre-increment value as result.
+    }
+  } else {
+    if (has_pre_inc_result) {
+      // Preserve as result.
+      __ pushl(EAX);  // Preserve value as result.
+    }
+    __ pushl(EDX);  // Preserve receiver.
+    __ pushl(EAX);  // Left operand.
+    __ pushl(one_value);  // Right operand.
+    const char* operator_name = (node->kind() == Token::kINCR) ? "+" : "-";
+    GenerateBinaryOperatorCall(node->operator_id(),
+                               node->token_index(),
+                               operator_name);
+    __ popl(EDX);  // Restore receiver.
+  }
+  // EAX: Result of binary operation.
+  // EDX: receiver
+  if (IsResultNeeded(node) && node->prefix()) {
+    // Value stored into field is the result.
+    __ pushl(EAX);
+  }
+
+  // TODO(srdjan): Inline instance setter.
+  __ pushl(EDX);  // Receiver.
+  __ pushl(EAX);  // Value.
+  // It is not necessary to generate a type test of the assigned value here,
+  // because the setter will check the type of its incoming arguments.
+  GenerateInstanceSetterCall(node->setter_id(),
+                             node->token_index(),
+                             node->field_name());
+}
+
+
+
+
+
 // Return offset of a field or -1 if field is not found.
 static intptr_t GetFieldOffset(const Class& field_class,
                                const String& field_name) {
@@ -1143,41 +1222,51 @@
 
 
 // Emits code for an instance getter that has one or more collected classes,
-// all with the same target.
+// all with the same target. Deoptimizes for Smi or unexpected class.
+//  EBX: loaded receiver.
+// Result is returned in EAX.
 void OptimizingCodeGenerator::InlineInstanceGettersWithSameTarget(
-    InstanceGetterNode* node, const Function& target) {
-  const ZoneGrowableArray<const Class*>* classes = CollectedClassesAtNode(node);
-  ASSERT(classes->length() > 0);
-  Label load_field;
-  VisitLoadOne(node->receiver(), EBX);
+    AstNode* node,
+    AstNode* receiver,
+    const String& field_name,
+    Register recv_reg) {
+  if (recv_reg != EBX) {
+    // TODO(srdjan): Do not hardwire register.
+    UNIMPLEMENTED();
+  }
   DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EBX);
-  if (NodeMayBeSmi(node->receiver())) {
+  if (NodeMayBeSmi(receiver)) {
     __ testl(EBX, Immediate(kSmiTagMask));
     __ j(ZERO, deopt_blob->label());
   }
+
   __ movl(EAX, FieldAddress(EBX, Object::class_offset()));
-  const int num_classes = classes->length();
-  for (intptr_t i = 0; i < num_classes; i++) {
-    const Class& cls = *(*classes)[i];
+  const ICData& ic_data = node->ICDataAtId(node->id());
+  Function& target = Function::Handle();
+  Label load_field;
+  for (intptr_t i = 0; i < ic_data.NumberOfChecks(); i++) {
+    Class& cls = Class::ZoneHandle();
+    ic_data.GetOneClassCheckAt(i, &cls, &target);
     __ CompareObject(EAX, cls);
-    if (i == (num_classes - 1)) {
+    if (i == (ic_data.NumberOfChecks() - 1)) {
       __ j(NOT_EQUAL, deopt_blob->label());
     } else {
       __ j(EQUAL, &load_field, Assembler::kNearJump);
     }
   }
+  Class& cls = Class::Handle();
+  ic_data.GetOneClassCheckAt(0, &cls, &target);
+
   __ Bind(&load_field);
-
   // EBX: receiver.
   if (target.kind() == RawFunction::kImplicitGetter) {
     TraceOpt(node, "Inlines instance getter with same target");
-    // Inlineable load field.
-    intptr_t field_offset = GetFieldOffset(*(*classes)[0],
-                                           node->field_name());
+    intptr_t field_offset = GetFieldOffset(cls, field_name);
     ASSERT(field_offset >= 0);
     __ movl(EAX, FieldAddress(EBX, field_offset));
     return;
   }
+
   Recognizer::Kind recognized_kind = Recognizer::RecognizeKind(target);
   switch (recognized_kind) {
     case Recognizer::kObjectArrayLength: {
@@ -1188,7 +1277,7 @@
     case Recognizer::kGrowableArrayLength: {
       TraceOpt(node, "Inlines GrowableObjectArray.length");
       intptr_t field_offset = GetFieldOffset(
-          *(*classes)[0],
+          cls,
           String::Handle(String::NewSymbol(kGrowableArrayLengthFieldName)));
       __ movl(EAX, FieldAddress(EBX, field_offset));
       return;
@@ -1213,62 +1302,70 @@
 }
 
 
-// TODO(srdjan): Implement for multiple getter targets.
-// For every class inline its implicit getter, or call the instance getter.
-void OptimizingCodeGenerator::VisitInstanceGetterNode(
-    InstanceGetterNode* node) {
-  const char* kMessage = "Inline instance getter";
-  const ZoneGrowableArray<const Class*>* classes = CollectedClassesAtNode(node);
-  const String& getter_name =
-      String::Handle(Field::GetterName(node->field_name()));
-  if (FLAG_trace_optimization) {
-    OS::Print("Getter %s ", getter_name.ToCString());
-  }
-  if ((classes == NULL) || classes->is_empty()) {
-    TraceNotOpt(node, kMessage);
-    CodeGenerator::VisitInstanceGetterNode(node);
-    return;
-  }
-  // Collect all targets and identify if they are all inlineable and
-  // all same. Use 'targets' for case when not all targets are inlineable.
-  const intptr_t num_classes = classes->length();
-  GrowableArray<const Function*> targets(num_classes);
-  bool all_inlineable = true;
-  bool all_same_target = true;
-  for (intptr_t i = 0; i < num_classes; i++) {
-    const Class& cls = *(*classes)[i];
-    const int kNumArguments = 1;
-    const int kNumNamedArguments = 0;
-    const Function& target = Function::ZoneHandle(
-        Resolver::ResolveDynamicForReceiverClass(cls,
-                                                 getter_name,
-                                                 kNumArguments,
-                                                 kNumNamedArguments));
+// Return true if all targets in 'ic_data' point to same
+// inlineable getter targe.

[regis, 2011/11/02 02:02:54]

target

[srdjan, 2011/11/02 15:26:20]

Done.

+bool OptimizingCodeGenerator::ICDataToSameInlineableInstanceGetter(
+    const ICData& ic_data) {
+  Function& prev_target = Function::Handle();
+  Function& target = Function::Handle();
+  Class& cls = Class::Handle();
+  for (intptr_t i = 0; i < ic_data.NumberOfChecks(); i++) {
+    ic_data.GetOneClassCheckAt(i, &cls, &target);
     ASSERT(!target.IsNull());
-    targets.Add(&target);
-    if (targets[0]->raw() != target.raw()) {
-      all_same_target = false;
+    if (!prev_target.IsNull() && (prev_target.raw() != target.raw())) {
+      return false;
     }
+    prev_target = target.raw();
     if (!IsInlineableInstanceGetter(target)) {
-      all_inlineable = false;
+      return false;
     }
   }
-  // TODO(srdjan): implement other variants.
-  if (all_inlineable && all_same_target) {
-    InlineInstanceGettersWithSameTarget(node, *targets[0]);
+  return true;
+}
+
+
+void OptimizingCodeGenerator::InlineInstanceGetter(AstNode* node,
+                                                   intptr_t id,
+                                                   AstNode* receiver,
+                                                   const String& field_name,
+                                                   Register recv_reg) {
+  if (ICDataToSameInlineableInstanceGetter(node->ICDataAtId(id))) {
+    InlineInstanceGettersWithSameTarget(node, receiver, field_name, recv_reg);
   } else {
     // TODO(srdjan): Inline access.
-    TraceNotOpt(node, kMessage);
-    node->receiver()->Visit(this);
+    __ pushl(recv_reg);
     const int kNumberOfArguments = 1;
     const Array& kNoArgumentNames = Array::Handle();
     GenerateCheckedInstanceCalls(node,
-                                 node->receiver(),
+                                 receiver,
                                  node->id(),
                                  node->token_index(),
                                  kNumberOfArguments,
                                  kNoArgumentNames);
   }
+}
+
+
+// TODO(srdjan): Implement for multiple getter targets.
+// For every class inline its implicit getter, or call the instance getter.
+void OptimizingCodeGenerator::VisitInstanceGetterNode(
+    InstanceGetterNode* node) {
+  const ICData& ic_data = node->ICDataAtId(node->id());
+  if (ic_data.NumberOfChecks() == 0) {
+    // No type feedback collected.
+    node->receiver()->Visit(this);
+    DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node);
+    __ jmp(deopt_blob->label());
+    return;
+  }
+
+  VisitLoadOne(node->receiver(), EBX);
+  InlineInstanceGetter(node,
+                       node->id(),
+                       node->receiver(),
+                       node->field_name(),
+                       EBX);
+  // Result is in EAX.
   if (CodeGenerator::IsResultNeeded(node)) {
     __ pushl(EAX);
   }
@@ -1770,12 +1867,12 @@
     return;
   }
 
-  if (NodeHasOnlyClass(node, smi_class_)) {
+  if (AtIdNodeHasOnlyClass(node, node->id(), smi_class_)) {
     if (GenerateSmiComparison(node)) {
       return;
     }
     // Fall through if condition is not supported.
-  } else if (NodeHasOnlyClass(node, double_class_)) {
+  } else if (AtIdNodeHasOnlyClass(node, node->id(), double_class_)) {
     // Double comparison
     if (GenerateDoubleComparison(node)) {
       return;
@@ -1799,12 +1896,13 @@
       Class::ZoneHandle(object_store->array_class());
   const Class& immutable_object_array_class =
       Class::ZoneHandle(object_store->immutable_array_class());
-  if (NodeHasOnlyClass(node, object_array_class) ||
-      NodeHasOnlyClass(node, immutable_object_array_class)) {
+  if (AtIdNodeHasOnlyClass(node, node->id(), object_array_class) ||
+      AtIdNodeHasOnlyClass(node, node->id(), immutable_object_array_class)) {
     VisitLoadTwo(node->array(), node->index_expr(), EBX, EDX);
     DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EBX, EDX);
-    const Class& test_class = NodeHasOnlyClass(node, object_array_class) ?
-        object_array_class : immutable_object_array_class;
+    const Class& test_class =
+        AtIdNodeHasOnlyClass(node, node->id(), object_array_class) ?
+            object_array_class : immutable_object_array_class;
     // Type checks of array.
     __ testl(EBX, Immediate(kSmiTagMask));  // Deoptimize if Smi.
     __ j(ZERO, deopt_blob->label());
@@ -1833,7 +1931,7 @@
   const Class& growable_array_class = Class::ZoneHandle(
       Library::Handle(Library::CoreImplLibrary()).
           LookupClass(growable_object_array_class_name));
-  if (NodeHasOnlyClass(node, growable_array_class)) {
+  if (AtIdNodeHasOnlyClass(node, node->id(), growable_array_class)) {
     const String& growable_array_length_field_name =
         String::Handle(String::NewSymbol(kGrowableArrayLengthFieldName));
     const String& growable_array_array_field_name =
@@ -1882,7 +1980,7 @@
   ObjectStore* object_store = Isolate::Current()->object_store();
   const Class& object_array_class =
       Class::ZoneHandle(object_store->array_class());
-  if (NodeHasOnlyClass(node, object_array_class)) {
+  if (AtIdNodeHasOnlyClass(node, node->id(), object_array_class)) {
     VisitLoadTwo(node->index_expr(), node->value(), EBX, ECX);
     DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EAX, EBX, ECX);
     __ popl(EAX);  // array.
@@ -2198,7 +2296,7 @@
           Recognizer::KindToCString(recognized));
     }
     if ((recognized == Recognizer::kIntegerToDouble) &&
-        NodeHasOnlyClass(node, smi_class_)) {
+        AtIdNodeHasOnlyClass(node, node->id(), smi_class_)) {
       // TODO(srdjan): Check if we could use temporary double instead of
       // allocating a new object every time.
       const Code& stub =
@@ -2217,7 +2315,7 @@
     }
 
     if ((recognized == Recognizer::kDoubleToDouble) &&
-        NodeHasOnlyClass(node, double_class_)) {
+        AtIdNodeHasOnlyClass(node, node->id(), double_class_)) {
       DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EAX);
       __ popl(EAX);
       CheckIfDoubleOrSmi(EAX, EBX, deopt_blob->label(), deopt_blob->label());