It does now :) On Thu, Oct 2, 2008 at 2:37 PM, Kasper Lund <[EMAIL PROTECTED]> wrote:
> 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 > > > > > > > -- Google Denmark ApS CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
