LGTM. Does it lint? On Thu, Oct 2, 2008 at 2:16 PM, <[EMAIL PROTECTED]> wrote: > Reviewers: Kasper Lund, > > Description: > In the code generator, avoid loading the arguments object to the > expression stack when it is already there. Also, cleanup up the > (two!) extra copies of the arguments object left on the stack. > > Please review this at http://codereview.chromium.org/5667 > > Affected files: > M src/codegen-arm.cc > M src/codegen-ia32.cc > > > Index: src/codegen-arm.cc > =================================================================== > --- src/codegen-arm.cc (revision 409) > +++ src/codegen-arm.cc (working copy) > @@ -576,22 +576,18 @@ > if (scope->arguments() != NULL) { > ASSERT(scope->arguments_shadow() != NULL); > Comment cmnt(masm_, "[ allocate arguments object"); > - { > - Reference target(this, scope->arguments()); > - __ ldr(r0, FunctionOperand()); > - __ push(r0); > - __ CallRuntime(Runtime::kNewArguments, 1); > - __ push(r0); > - SetValue(&target); > + { Reference shadow_ref(this, scope->arguments_shadow()); > + { Reference arguments_ref(this, scope->arguments()); > + > + __ ldr(r0, FunctionOperand()); > + __ push(r0); > + __ CallRuntime(Runtime::kNewArguments, 1); > + __ push(r0); > + SetValue(&arguments_ref); > + } > + SetValue(&shadow_ref); > } > - // The value of arguments must also be stored in .arguments. > - // TODO(1241813): This code can probably be improved by fusing it > with > - // the code that stores the arguments object above. > - { > - Reference target(this, scope->arguments_shadow()); > - Load(scope->arguments()); > - SetValue(&target); > - } > + __ pop(r0); // Value is no longer needed. > } > > // Generate code to 'execute' declarations and initialize > Index: src/codegen-ia32.cc > =================================================================== > --- src/codegen-ia32.cc (revision 409) > +++ src/codegen-ia32.cc (working copy) > @@ -661,21 +661,27 @@ > ASSERT(scope->arguments() != NULL); > ASSERT(scope->arguments_shadow() != NULL); > Comment cmnt(masm_, "[ store arguments object"); > - { > - Reference target(this, scope->arguments()); > - if (!arguments_object_saved) { > - __ push(Operand(ecx)); > + { Reference shadow_ref(this, scope->arguments_shadow()); > + { Reference arguments_ref(this, scope->arguments()); > + // If the newly-allocated arguments object is already on the > + // stack, we make use of the property that references > representing > + // variables take up no space on the expression stack (ie, it > + // doesn't matter that the stored value is actually below the > + // reference). > + ASSERT(arguments_ref.size() == 0); > + ASSERT(shadow_ref.size() == 0); > + > + // If the newly-allocated argument object is not already on the > + // stack, we rely on the property that loading a > + // (zero-sized) reference will not clobber the ecx register. > + if (!arguments_object_saved) { > + __ push(ecx); > + } > + SetValue(&arguments_ref); > } > - SetValue(&target); > + SetValue(&shadow_ref); > } > - // The value of arguments must also be stored in .arguments. > - // TODO(1241813): This code can probably be improved by fusing it > with > - // the code that stores the arguments object above. > - { > - Reference target(this, scope->arguments_shadow()); > - Load(scope->arguments()); > - SetValue(&target); > - } > + __ pop(eax); // Value is no longer needed. > } > > // Generate code to 'execute' declarations and initialize > > >
--~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
