I really like the cleanup of the virtual frame CallXXX functions. I think it pays to be more explicit there and at the same time offload some of the setup from within the code generator.
I'm not sure about the unspilled scope in the middle of object literals. And I would like to see the spilled scopes pushed into the actual code generation functions of the inlined builtins (even if it's only temporary), to increase our confidence in this change based on testing. http://codereview.chromium.org/18596/diff/402/601 File src/codegen-ia32.cc (right): http://codereview.chromium.org/18596/diff/402/601#newcode3006 Line 3006: // Beginning of unspilled code, falls through to the function return. This seems wrong, or at least weird. Manually setting the spilled code flag is a bad smell. All the frame and codegen functions should work if they get a frame that happens to be spilled, so it seems unnecessary. It also seems wrong to force ourselves into an unspilled state with a live spilled scope in the current C++ frame. And it really seems scary that since this code is in a loop and the other cases all expect a spilled frame (all the raw accesses to the stack top and raw pushes), we can be leaving ourselves in an inconsistent state for the remaining iterations. http://codereview.chromium.org/18596/diff/402/601#newcode3713 Line 3713: VirtualFrame::SpilledScope spilled_scope(this); You should remove this spilled scope from here and add it to *all* the inline runtime calls (the GenerateXXX functions for inline runtime calls, like you did in GenerateIsArray just above). Spilling before checking for an inlined call is almost the same as spilling for every runtime call. I'm concerned that the following unspilled code isn't actually getting enough interesting frames from our tests to have confidence in their coverage. http://codereview.chromium.org/18596/diff/402/601#newcode3748 Line 3748: &num_args, arg_count + 1); Indentation seems off. http://codereview.chromium.org/18596/diff/402/601#newcode4537 Line 4537: void Reference::GetValue(TypeofState typeof_state) { Since we are allocating explicit registers (eg, eax, ecx) in this snippet, and that won't work if we have off-frame register references, we should assert that we have valid entry registers, here and in SetValue. http://codereview.chromium.org/18596/diff/402/601#newcode4565 Line 4565: __ mov(ecx, name); __ mov(name_reg.reg(), name); http://codereview.chromium.org/18596/diff/402/601#newcode4568 Line 4568: frame->CallCodeObject(ic, RelocInfo::CODE_TARGET_CONTEXT, &name_reg, 0); Since the two alternatives here only differ by their relocation info, it might be cleaner to set a relocation info variable and then have only a single call to the code object. Up to you. http://codereview.chromium.org/18596/diff/402/601#newcode4572 Line 4572: frame->EmitPush(eax); // IC call leaves result in eax, push it out Push the result of the call (rather than discarding it), not literally eax. And remove the comment. http://codereview.chromium.org/18596/diff/402/601#newcode4669 Line 4669: answer = frame->CallCodeObject(ic, Same comment about cases that only differ in one subexpression. http://codereview.chromium.org/18596/diff/402/601#newcode4680 Line 4680: frame->Push(&answer); // IC call leaves result in eax, push it out Remove this comment. http://codereview.chromium.org/18596/diff/402/602 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/18596/diff/402/602#newcode261 Line 261: void VirtualFrame::PrepareForCall(int dropped_args, int spilled_args) { Nothing will break in the implementation, but it seems like it might be an error for a callee to pop more arguments than it was passed. If so, here might be a good place to assert that dropped_args <= spilled_args. http://codereview.chromium.org/18596/diff/402/602#newcode956 Line 956: default: It's not entirely obvious why the other code kinds cannot reach here (ie, they are called with register arguments). This needs a comment at the default case or before the switch. http://codereview.chromium.org/18596/diff/402/602#newcode984 Line 984: default: Ditto. http://codereview.chromium.org/18596/diff/402/602#newcode1012 Line 1012: default: Ditto. http://codereview.chromium.org/18596/diff/402/603 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/18596/diff/402/603#newcode210 Line 210: void SpillTopElements(int number_spilled); Since this doesn't appear to be called anywhere in this changelist, I would get rid of it. If there's a reason to keep it, does it need to be public? It seems like an implementaiton utility, not something to expose in the public API. http://codereview.chromium.org/18596/diff/402/603#newcode369 Line 369: // The optional arguments passed in as Result pointers are Unused() from I wouldn't document the (current) implementation so much in the header comments. Say something like "Register arguments are passed as results and consumed by the call." http://codereview.chromium.org/18596/diff/402/603#newcode538 Line 538: // the frame when the call returns. Spill all elements in registers. The comment wording is weird. It sounds like registers might be spilled after the call. Please reword. http://codereview.chromium.org/18596/diff/402/603#newcode561 Line 561: // Calls a code object, dropping dropped_args arguments from the "Calls a code object which takes spilled_args frame arguments, of which dropped_args are popped by the callee." It seems more natural to me to put spilled_args before dropped_args in the signature. http://codereview.chromium.org/18596 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
