I just started looking at this. Here are a few quick comments, I'll have more later.
http://codereview.chromium.org/17380/diff/5/6 File src/codegen-ia32.cc (right): http://codereview.chromium.org/17380/diff/5/6#newcode753 Line 753: // The arguments to the binary operation are passed in on the frame (stack). Unclear. They may or may not be on the stack at run time, and there is no stack at compile time. "The arguments are on top of the code generator's frame." http://codereview.chromium.org/17380/diff/5/6#newcode763 Line 763: Result eax_reg = generator()->allocator()->Allocate(eax); Allocate the result in VirtualFrame::CallStub and return it, rather than at every call site. That gives a single place to change it rather than a bunch. Just call it "result" or something. Even though it might be eax, we don't care here. http://codereview.chromium.org/17380/diff/5/6#newcode813 Line 813: // TODO(): When this code is updated to use the virtual frame, it Remove this comment. http://codereview.chromium.org/17380/diff/5/6#newcode819 Line 819: // The operands are on the stack. Frame? I think it's obvious anyway. http://codereview.chromium.org/17380/diff/5/6#newcode4784 Line 4784: // This function's implementation is a copy of This comment is no longer true as of this change. Please update. http://codereview.chromium.org/17380/diff/5/6#newcode4805 Line 4805: Result right = frame->Pop(); // Get the right input to the operation. The comment doesn't seem to say any more (in fact, it says less) than the code itself. http://codereview.chromium.org/17380/diff/5/6#newcode4809 Line 4809: // Temp is used to compute the answer, leaving left and right unchanged. Then it's not just "temp". Call it result or answer, maybe? http://codereview.chromium.org/17380/diff/5/6#newcode4842 Line 4842: // Remove tag from one of the operands (but keep sign). Not just one of the arguments, but the left one. http://codereview.chromium.org/17380/diff/5/6#newcode4844 Line 4844: // We no longer require that the destination be eax. Document the code as currently written, not what we used to do. This comment makes little sense to the reader. http://codereview.chromium.org/17380/diff/5/6#newcode4853 Line 4853: Label non_zero_product_mul; Comment why it's safe to have a raw Label here (no frame effect on either branch). http://codereview.chromium.org/17380/diff/5/8 File src/jump-target-ia32.cc (right): http://codereview.chromium.org/17380/diff/5/8#newcode296 Line 296: ASSERT(!had_entry_frame || arg->type() == arg_type); This ASSERT should go too (there is no reason that a register stays in a register or a memory stays in a memory at a bind site). Updating your workspace should actually get rid of it anyway. http://codereview.chromium.org/17380 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
