Optional arguments in new compiler.

runtime/vm/code_generator_ia32.cc

Index: runtime/vm/code_generator_ia32.cc
===================================================================
--- runtime/vm/code_generator_ia32.cc	(revision 5369)
+++ runtime/vm/code_generator_ia32.cc	(working copy)
@@ -356,44 +356,13 @@
   LocalScope* scope = parsed_function_.node_sequence()->scope();
   const int num_fixed_params = function.num_fixed_parameters();
   const int num_opt_params = function.num_optional_parameters();
-  const int num_params = num_fixed_params + num_opt_params;
-  int first_param_index;
-  int first_local_index;
-  int num_copied_params;
-  // Assign indices to parameters and locals.
-  if (num_params == num_fixed_params) {
-    // No need to copy incoming arguments.
-    // The body of the function will access parameter i at fp[1 + num_fixed - i]
-    // and local variable j at fp[-1 - j].
-    first_param_index = 1 + num_params;
-    first_local_index = -1;
-    num_copied_params = 0;
-  } else {
-    // The body of the function will access copied parameter i at fp[-1 - i]
-    // and local j at fp[-1 - num_params - j].
-    first_param_index = -1;
-    first_local_index = -1 - num_params;
-    num_copied_params = num_params;
-    ASSERT(num_copied_params > 0);
-  }
+  const int num_copied_params = parsed_function_.copied_parameter_count();
+  const int stack_slot_count = parsed_function_.local_count();
+  set_locals_space_size(stack_slot_count * kWordSize);
 
-  // Allocate parameters and local variables, either in the local frame or in
-  // the context(s).
-  LocalScope* context_owner = NULL;  // No context needed so far.
-  int first_free_frame_index =
-      scope->AllocateVariables(first_param_index,
-                               num_params,
-                               first_local_index,
-                               scope,  // Initial loop owner.
-                               &context_owner);
-  // Frame indices are relative to the frame pointer and are decreasing.
-  ASSERT(first_free_frame_index <= first_local_index);
-  const int num_locals = first_local_index - first_free_frame_index;
-
   // Reserve local space for copied incoming and default arguments and locals.
   // TODO(regis): We may give up reserving space on stack for args/locals
   // because pushes of initial values may be more effective than moves.
-  set_locals_space_size((num_copied_params + num_locals) * kWordSize);
   __ EnterFrame(locals_space_size());
 
   // We check the number of passed arguments when we have to copy them due to
@@ -401,6 +370,7 @@
   // No such checking code is generated if only fixed parameters are declared,
   // unless we are debug mode or unless we are compiling a closure.
   if (num_copied_params == 0) {
+    ASSERT(num_opt_params == 0);
 #if defined(DEBUG)
     const bool check_arguments = true;  // Always check arguments in debug mode.
 #else
@@ -414,14 +384,8 @@
       Label argc_in_range;
       // Total number of args is the first Smi in args descriptor array (EDX).
       __ movl(EAX, FieldAddress(EDX, Array::data_offset()));
-      if (num_opt_params == 0) {
-        __ cmpl(EAX, Immediate(Smi::RawValue(num_fixed_params)));
-        __ j(EQUAL, &argc_in_range, Assembler::kNearJump);
-      } else {
-        __ subl(EAX, Immediate(Smi::RawValue(num_fixed_params)));
-        __ cmpl(EAX, Immediate(Smi::RawValue(num_opt_params)));
-        __ j(BELOW_EQUAL, &argc_in_range, Assembler::kNearJump);
-      }
+      __ cmpl(EAX, Immediate(Smi::RawValue(num_fixed_params)));
+      __ j(EQUAL, &argc_in_range, Assembler::kNearJump);
       if (function.IsClosureFunction()) {
         GenerateCallRuntime(AstNode::kNoId,
                             0,
@@ -432,11 +396,12 @@
       __ Bind(&argc_in_range);
     }
   } else {
-    ASSERT(first_param_index == -1);
+    ASSERT(parsed_function_.first_parameter_index() == -1);
     // Copy positional arguments.
     // Check that no fewer than num_fixed_params positional arguments are passed
     // in and that no more than num_params arguments are passed in.
     // Passed argument i at fp[1 + argc - i] copied to fp[-1 - i].
+    const int num_params = num_fixed_params + num_opt_params;
 
     // Total number of args is the first Smi in args descriptor array (EDX).
     __ movl(EBX, FieldAddress(EDX, Array::data_offset()));
@@ -606,11 +571,12 @@
   // TODO(regis): For now, always unroll the init loop. Decide later above
   // which threshold to implement a loop.
   // Consider emitting pushes instead of moves.
-  for (int index = first_local_index; index > first_free_frame_index; index--) {
-    if (index == first_local_index) {
+  const int base = parsed_function_.first_local_index();

[Kevin Millikin (Google), 2012/03/13 12:31:08]

I think with your change to make local_count() include the number of copied
parameters, you will want something like:

const int base = parsed_function_.first_local_index();
const int count =
  parsed_function_.local_count() - parsed_function_.copied_parameter_count();
for (int index = 0; index < count; ++index) ...

Otherwise we're needlessly initializing above the stack pointer (with an
unrolled loop!).

[srdjan, 2012/03/13 16:57:00]

Excluding num_copied_parameters from stack_local_count.

+  for (int index = 0; index < parsed_function_.local_count(); ++index) {
+    if (index == 0) {
       __ movl(EAX, raw_null);
     }
-    __ movl(Address(EBP, index * kWordSize), EAX);
+    __ movl(Address(EBP, (base - index) * kWordSize), EAX);
   }
 
   // Generate stack overflow check.
@@ -765,19 +731,20 @@
 
 void CodeGenerator::VisitClosureNode(ClosureNode* node) {
   const Function& function = node->function();
-  if (function.IsNonImplicitClosureFunction()) {
+  if (function.IsNonImplicitClosureFunction() &&
+      (function.context_scope() == ContextScope::null())) {
     const int current_context_level = state()->context_level();
     const ContextScope& context_scope = ContextScope::ZoneHandle(
         node->scope()->PreserveOuterScope(current_context_level));
     ASSERT(!function.HasCode());
-    ASSERT(function.context_scope() == ContextScope::null());
     function.set_context_scope(context_scope);
   } else {
-    ASSERT(function.context_scope() != ContextScope::null());
     if (function.IsImplicitInstanceClosureFunction()) {
       node->receiver()->Visit(this);
     }
   }
+  ASSERT(function.context_scope() != ContextScope::null());
+
   // The function type of a closure may have type arguments. In that case, pass
   // the type arguments of the instantiator.
   const Class& cls = Class::Handle(function.signature_class());