Aside from my comments, it looks good. But Ivan should look at it, I think.
http://codereview.chromium.org/10993/diff/1/5 File src/codegen-ia32.cc (right): http://codereview.chromium.org/10993/diff/1/5#newcode2600 Line 2600: // Smis are loaded in two steps via a temporary register. Is now the time for special-casing small literals, and 0? Load 0 by xor, and load literals between -255 and 255, or 16-bit literals, specially? I think this would hit a majority of code literals. http://codereview.chromium.org/10993/diff/1/3 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/10993/diff/1/3#newcode350 Line 350: void VirtualFrame::StoreToFrameSlotAt(int index) { So this changelist enables the register and memory types, and also the constant type, since there is no extra processing for the constant type? Should it be commented that this correctly handles constants, since there is no explicit mention of it in the code? http://codereview.chromium.org/10993/diff/1/3#newcode358 Line 358: elements_[index] = top; Don't we need to unuse elements_[index] if it is a register here? http://codereview.chromium.org/10993/diff/1/3#newcode363 Line 363: ASSERT(!tmp.is(no_reg)); Will we actually need error handling code in the future, to deal with the case that the allocator fails? Or did we decide that the allocator must always be able to succeed, by spilling, because we will never have all regs externally used? http://codereview.chromium.org/10993/diff/1/4 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/10993/diff/1/4#newcode73 Line 73: type_ = (type_ & ~SyncField::mask()) | SyncField::encode(SYNCED); Why isn't there a set_field accessor for bit fields? Is this the way we always set bit fields? http://codereview.chromium.org/10993 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
