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. On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/601#newcode3713 Line 3713: VirtualFrame::SpilledScope spilled_scope(this); On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/601#newcode3748 Line 3748: &num_args, arg_count + 1); On 2009/01/29 09:11:07, Kevin Millikin wrote: > Indentation seems off. Done. http://codereview.chromium.org/18596/diff/402/601#newcode4537 Line 4537: void Reference::GetValue(TypeofState typeof_state) { On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/601#newcode4565 Line 4565: __ mov(ecx, name); On 2009/01/29 09:11:07, Kevin Millikin wrote: > __ mov(name_reg.reg(), name); Done. http://codereview.chromium.org/18596/diff/402/601#newcode4568 Line 4568: frame->CallCodeObject(ic, RelocInfo::CODE_TARGET_CONTEXT, &name_reg, 0); On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/601#newcode4572 Line 4572: frame->EmitPush(eax); // IC call leaves result in eax, push it out On 2009/01/29 09:11:07, Kevin Millikin wrote: > Push the result of the call (rather than discarding it), not literally eax. > And remove the comment. Done. http://codereview.chromium.org/18596/diff/402/601#newcode4669 Line 4669: answer = frame->CallCodeObject(ic, On 2009/01/29 09:11:07, Kevin Millikin wrote: > Same comment about cases that only differ in one subexpression. Done. http://codereview.chromium.org/18596/diff/402/601#newcode4680 Line 4680: frame->Push(&answer); // IC call leaves result in eax, push it out On 2009/01/29 09:11:07, Kevin Millikin wrote: > Remove this comment. Done. 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) { On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/602#newcode956 Line 956: default: On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/602#newcode984 Line 984: default: On 2009/01/29 09:11:07, Kevin Millikin wrote: > Ditto. Done. http://codereview.chromium.org/18596/diff/402/602#newcode1012 Line 1012: default: On 2009/01/29 09:11:07, Kevin Millikin wrote: > Ditto. Done. 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); On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/18596/diff/402/603#newcode369 Line 369: // The optional arguments passed in as Result pointers are Unused() from On 2009/01/29 09:11:07, Kevin Millikin wrote: > 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." Done. http://codereview.chromium.org/18596/diff/402/603#newcode538 Line 538: // the frame when the call returns. Spill all elements in registers. On 2009/01/29 09:11:07, Kevin Millikin wrote: > The comment wording is weird. It sounds like registers might be spilled after > the call. Please reword. Done. http://codereview.chromium.org/18596/diff/402/603#newcode561 Line 561: // Calls a code object, dropping dropped_args arguments from the On 2009/01/29 09:11:07, Kevin Millikin wrote: > "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. Done. http://codereview.chromium.org/18596 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
