Restructure instance setter and finish the last 1/3 of increment instance field optimization.

runtime/vm/opt_code_generator_ia32.cc

 // Copyright (c) 2011, 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/globals.h"  // Needed here to get TARGET_ARCH_IA32.
 #if defined(TARGET_ARCH_IA32)
 
 #include "vm/opt_code_generator.h"
 
 #include "vm/assembler_macros.h"
@@ skipping 271 matching lines @@
   for (intptr_t i = 0; i < ic_data.NumberOfChecks(); i++) {
     Class& cls = Class::ZoneHandle();
     ic_data.GetOneClassCheckAt(i, &cls, &target);
     result->Add(&cls);
   }
   return result;
 }
 
 
 // Debugging helper function.
-void OptimizingCodeGenerator::PrintCollectedClasses(AstNode* node) {
-  const ZoneGrowableArray<const Class*>* classes = CollectedClassesAtNode(node);
-  for (intptr_t i = 0; i < classes->length(); i++) {
-    OS::Print("- %s\n", (*classes)[i]->ToCString());
+void OptimizingCodeGenerator::PrintCollectedClassesAtId(AstNode* node,
+                                                        intptr_t id) {
+  const ICData& ic_data = node->ICDataAtId(id);
+  ASSERT(ic_data.NumberOfArgumentsChecked() == 1);
+  Function& target = Function::Handle();
+  Class& cls = Class::Handle();
+  for (intptr_t i = 0; i < ic_data.NumberOfChecks(); i++) {
+    ic_data.GetOneClassCheckAt(i, &cls, &target);
+    OS::Print("- %s -> %s\n", cls.ToCString(),
+        target.ToFullyQualifiedCString());
   }
 }
 
 
 void OptimizingCodeGenerator::TraceOpt(AstNode* node, const char* message) {
   if (FLAG_trace_optimization) {
     OS::Print("Opt node ix: %d; %s\n", node->token_index(), message);
   }
 }
 
@@ skipping 795 matching lines @@
     }
     if (node->prefix()) {
       __ pushl(EAX);
     } else {
       __ pushl(ECX);
     }
   }
 }
 
 
+// Debugging helper method, used in assert only.
+static bool HaveSameClassesInICData(const ICData& a, const ICData& b) {
+  if (a.NumberOfChecks() != b.NumberOfChecks()) {
+    return false;
+  }
+  if (a.NumberOfChecks() == 0) {
+    return true;
+  }
+  if (a.NumberOfArgumentsChecked() != b.NumberOfArgumentsChecked()) {
+    return false;
+  }
+  // Only one-argument checks implemented.
+  ASSERT(a.NumberOfArgumentsChecked() == 1);
+  Function& a_target = Function::Handle();
+  Function& b_target = Function::Handle();
+  Class& a_class = Class::Handle();
+  Class& b_class = Class::Handle();
+  for (int i = 0; i < a.NumberOfChecks(); i++) {

[siva, 2011/11/03 17:02:29]

In the loop above (printCollectedClassesAtId(...) the for loop uses intptr_t i,
I guess here too it should be intptr_t

[srdjan, 2011/11/03 18:36:13]

Done.

+    a.GetOneClassCheckAt(i, &a_class, &a_target);
+    bool found = false;
+    for (int n = 0; n < b.NumberOfChecks(); n++) {

[siva, 2011/11/03 17:02:29]

Ditto comment regarding intptr_t

[srdjan, 2011/11/03 18:36:13]

Done.

+      b.GetOneClassCheckAt(n, &b_class, &b_target);
+      if ((a_class.raw() == b_class.raw())) {
+        found = true;
+        break;
+      }
+    }
+    if (!found) {
+      return false;
+    }
+  }
+  return true;
+}
+
+
 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).
   const ICData& ic_data = node->ICDataAtId(node->id());
   if (ic_data.NumberOfChecks() == 0) {
     // 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, EBX);
@@ skipping 45 matching lines @@
                                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());
+  // This can never deoptimize since the checks are the same as in getter.
+  ASSERT(HaveSameClassesInICData(node->ICDataAtId(node->getter_id()),
+                                 node->ICDataAtId(node->setter_id())));
+  InlineInstanceSetter(node,
+                       node->setter_id(),
+                       node->receiver(),
+                       node->field_name(),
+                       EDX,   // receiver
+                       EAX);  // value.
 }
 
 
 
 
 
 // Return offset of a field or -1 if field is not found.
 static intptr_t GetFieldOffset(const Class& field_class,
                                const String& field_name) {
   Class& cls = Class::Handle(field_class.raw());
@@ skipping 35 matching lines @@
   return true;
 }
 
 
 // Emits code for an instance getter that has one or more collected classes,
 // all with the same target. Deoptimizes for Smi or unexpected class.
 //  EBX: loaded receiver.
 // Result is returned in EAX.
 void OptimizingCodeGenerator::InlineInstanceGettersWithSameTarget(
     AstNode* node,
+    intptr_t id,
     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(receiver)) {
     __ testl(EBX, Immediate(kSmiTagMask));
     __ j(ZERO, deopt_blob->label());
   }
 
   __ movl(EAX, FieldAddress(EBX, Object::class_offset()));
-  const ICData& ic_data = node->ICDataAtId(node->id());
+  const ICData& ic_data = node->ICDataAtId(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 == (ic_data.NumberOfChecks() - 1)) {
       __ j(NOT_EQUAL, deopt_blob->label());
     } else {
       __ j(EQUAL, &load_field, Assembler::kNearJump);
@@ skipping 23 matching lines @@
       TraceOpt(node, "Inlines GrowableObjectArray.length");
       intptr_t field_offset = GetFieldOffset(
           cls,
           String::Handle(String::NewSymbol(kGrowableArrayLengthFieldName)));
       __ movl(EAX, FieldAddress(EBX, field_offset));
       return;
     }
     default:
       UNIMPLEMENTED();
   }
+  UNREACHABLE();
 }
 
 
-bool OptimizingCodeGenerator::IsInlineableInstanceGetter(
-    const Function& function) {
+static bool IsInlineableInstanceGetter(const Function& function) {
   if (function.kind() == RawFunction::kImplicitGetter) {
     return true;
   }
   Recognizer::Kind recognized = Recognizer::RecognizeKind(function);
   if ((recognized == Recognizer::kObjectArrayLength) ||
       (recognized == Recognizer::kGrowableArrayLength)) {
     return true;
   }
   return false;
 }
 
 
-// Return true if all targets in 'ic_data' point to same
-// inlineable getter target.
-bool OptimizingCodeGenerator::ICDataToSameInlineableInstanceGetter(
-    const ICData& ic_data) {
+// Return the unique target of all checks or null.
+static RawFunction* GetUniqueTarget(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());
     if (!prev_target.IsNull() && (prev_target.raw() != target.raw())) {
-      return false;
+      return Function::null();
     }
     prev_target = target.raw();
-    if (!IsInlineableInstanceGetter(target)) {
-      return false;
-    }
   }
-  return true;
+  return target.raw();
+}
+
+
+// Return true if all targets in 'ic_data' point to same
+// inlineable getter target.
+static bool ICDataToSameInlineableInstanceGetter(const ICData& ic_data) {
+  const Function& target = Function::Handle(GetUniqueTarget(ic_data));
+  return !target.IsNull() && IsInlineableInstanceGetter(target);
+}
+
+
+// Return true if all targets in 'ic_data' point to same
+// inlineable getter target.
+static bool ICDataToSameInlineableInstanceSetter(const ICData& ic_data) {
+  const Function& target = Function::Handle(GetUniqueTarget(ic_data));
+  return !target.IsNull() && (target.kind() == RawFunction::kImplicitSetter);
 }
 
 
 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);
+    InlineInstanceGettersWithSameTarget(node,
+                                        id,
+                                        receiver,
+                                        field_name,
+                                        recv_reg);
   } else {
     // TODO(srdjan): Inline access.
     __ pushl(recv_reg);
     const int kNumberOfArguments = 1;
     const Array& kNoArgumentNames = Array::Handle();
     GenerateCheckedInstanceCalls(node,
                                  receiver,
-                                 node->id(),
+                                 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(
@@ skipping 13 matching lines @@
                        node->receiver(),
                        node->field_name(),
                        EBX);
   // Result is in EAX.
   if (CodeGenerator::IsResultNeeded(node)) {
     __ pushl(EAX);
   }
 }
 
 
+// Returns stored value in 'value_reg'.

[siva, 2011/11/03 17:02:29]

Should the comment say 'recv_reg' and 'value_reg' values are not clobbered.
EBX register is clobbered.

[srdjan, 2011/11/03 18:36:13]

Added Clobber EBX, 'value_reg' untouched.
Don't care about recv_reg at the moment. 

+void OptimizingCodeGenerator::InlineInstanceSettersWithSameTarget(
+    AstNode* node,
+    intptr_t id,
+    AstNode* receiver,
+    const String& field_name,
+    Register recv_reg,
+    Register value_reg) {
+  if ((recv_reg != EDX) || (value_reg != EAX)) {
+    // TODO(srdjan): Do not hardwire register.
+    UNIMPLEMENTED();
+  }

[siva, 2011/11/03 17:02:29]

Instead of this unimplemented why not just assert
recv_reg != EBX and value_reg != EBX and use
recv_reg in place of EDX and value_reg in place of EAX in
the code below?

[srdjan, 2011/11/03 18:36:13]

Done.

+  DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EDX, EAX);
+  if (NodeMayBeSmi(receiver)) {
+    __ testl(EDX, Immediate(kSmiTagMask));
+    __ j(ZERO, deopt_blob->label());
+  }
+  __ movl(EBX, FieldAddress(EDX, Object::class_offset()));
+  const ICData& ic_data = node->ICDataAtId(id);
+  Function& target = Function::Handle();
+  Label store_field;
+  for (intptr_t i = 0; i < ic_data.NumberOfChecks(); i++) {
+    Class& cls = Class::ZoneHandle();
+    ic_data.GetOneClassCheckAt(i, &cls, &target);
+    __ CompareObject(EBX, cls);
+    if (i == (ic_data.NumberOfChecks() - 1)) {
+      __ j(NOT_EQUAL, deopt_blob->label());
+    } else {
+      __ j(EQUAL, &store_field, Assembler::kNearJump);
+    }
+  }
+  Class& cls = Class::Handle();
+  ic_data.GetOneClassCheckAt(0, &cls, &target);
+
+  __ Bind(&store_field);
+  // EDX: receiver.
+  // EAX: value
+  ASSERT(target.kind() == RawFunction::kImplicitSetter);
+  intptr_t field_offset = GetFieldOffset(cls, field_name);
+  ASSERT(field_offset >= 0);
+  __ StoreIntoObject(EDX, FieldAddress(EDX, field_offset), EAX);
+}
+
+
+// Returns value in 'value_reg'.
+void OptimizingCodeGenerator::InlineInstanceSetter(AstNode* node,
+                                                   intptr_t id,
+                                                   AstNode* receiver,
+                                                   const String& field_name,
+                                                   Register recv_reg,
+                                                   Register value_reg) {
+  if (ICDataToSameInlineableInstanceSetter(node->ICDataAtId(id))) {
+    InlineInstanceSettersWithSameTarget(node,
+                                        id,
+                                        receiver,
+                                        field_name,
+                                        recv_reg,
+                                        value_reg);
+  } else {
+    UNIMPLEMENTED();
+  }
+}
+
+
 // The call to the instance setter implements the assignment to a field.
 // The result of the assignment to a field is the value being stored.
 void OptimizingCodeGenerator::VisitInstanceSetterNode(
     InstanceSetterNode* node) {
-  if (FLAG_enable_type_checks) {
+  // TODO(srdjan): inline setters to different targets as well.
+  if (FLAG_enable_type_checks ||
+      !ICDataToSameInlineableInstanceSetter(node->ICDataAtId(node->id()))) {
     CodeGenerator::VisitInstanceSetterNode(node);
     return;
   }
-  const char* kMessage = "Inline instance setter";
-  const ZoneGrowableArray<const Class*>* classes = CollectedClassesAtNode(node);
-  if ((classes == NULL) || classes->is_empty()) {
-    // Type feedback not yet collected.
-    TraceNotOpt(node, kMessage);
-    CodeGenerator::VisitInstanceSetterNode(node);
+  VisitLoadTwo(node->receiver(), node->value(), EDX, EAX);
+  const ICData& ic_data = node->ICDataAtId(node->id());
+  if (ic_data.NumberOfChecks() == 0) {
+    DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EDX, EAX);
+    __ jmp(deopt_blob->label());
     return;
   }
-  const int num_classes = classes->length();
-  bool all_inlineable = true;
-  const String& setter_name =
-      String::Handle(Field::SetterName(node->field_name()));
-  if (FLAG_trace_optimization) {
-    OS::Print("Setter: %s ", setter_name.ToCString());
-  }
-  // TODO(srdjan): Replace simple heuristic expecting that all setters
-  // for a call-site must be inlineable or none will be inlined.
-  for (intptr_t i = 0; i < num_classes; i++) {
-    const Class& cls = *(*classes)[i];
-    const int kNumArguments = 2;
-    const int kNumNamedArguments = 0;
-    const Function& target = Function::ZoneHandle(
-        Resolver::ResolveDynamicForReceiverClass(cls,
-                                                 setter_name,
-                                                 kNumArguments,
-                                                 kNumNamedArguments));
-    ASSERT(!target.IsNull());
-    if (target.kind() != RawFunction::kImplicitSetter) {
-      all_inlineable = false;
-      break;
-    }
-  }
+  // Value in EAX survives and will be stored on stack if result is needed.
+  InlineInstanceSetter(node,
+                       node->id(),
+                       node->receiver(),
+                       node->field_name(),
+                       EDX,
+                       EAX);
 
-  // TODO(srdjan): Add an upper limit to number of class tests.
-  if ((classes == NULL) || (num_classes == 0) || !all_inlineable) {
-    TraceNotOpt(node, kMessage);
-    CodeGenerator::VisitInstanceSetterNode(node);
-    return;
-  }
-
-  TraceOpt(node, kMessage);
-  // Inline setter(s).
-  VisitLoadTwo(node->receiver(), node->value(), EAX, EDX);
-  DeoptimizationBlob* deopt_blob = AddDeoptimizationBlob(node, EAX, EDX);
-  // Smi causes deoptimization.
-  if (NodeMayBeSmi(node->receiver())) {
-    __ testl(EAX, Immediate(kSmiTagMask));
-    __ j(ZERO, deopt_blob->label());
-  }
-  __ movl(EBX, FieldAddress(EAX, Object::class_offset()));
-  // EAX: receiver, EBX: receiver's class, EDX: value.
-
-
-  Label done;
-  for (int i = 0; i < num_classes; i++) {
-    intptr_t field_offset = GetFieldOffset(*(*classes)[i],
-                                           node->field_name());
-    ASSERT(field_offset >= 0);
-    const Class& cls = *(*classes)[i];
-    __ CompareObject(EBX, cls);
-    if (i != (num_classes - 1)) {
-      Label next_test;
-      __ j(NOT_EQUAL, &next_test);
-      __ StoreIntoObject(EAX, FieldAddress(EAX, field_offset), EDX);
-      __ jmp(&done);
-      __ Bind(&next_test);
-    } else {
-      // If last check fails deoptimize, otherwise store and fall through.
-      __ j(NOT_EQUAL, deopt_blob->label());
-      __ StoreIntoObject(EAX, FieldAddress(EAX, field_offset), EDX);
-    }
-  }
-  __ Bind(&done);
   if (CodeGenerator::IsResultNeeded(node)) {
-    __ pushl(EDX);
+    __ pushl(EAX);
   }
 }
 
 
-
 // Return false if condition is not supported.
 static bool SupportedTokenKindToSmiCondition(Token::Kind kind,
                                              Condition* condition) {
   switch (kind) {
     case Token::kEQ:
       *condition = EQUAL;
       return true;
     case Token::kNE:
       *condition = NOT_EQUAL;
       return true;
@@ skipping 896 matching lines @@
   __ addl(ESP, Immediate(node->arguments()->length() * kWordSize));
   // Result is in EAX.
   if (IsResultNeeded(node)) {
     __ pushl(EAX);
   }
 }
 
 }  // namespace dart
 
 #endif  // defined TARGET_ARCH_IA32