Here is a revised version.
http://codereview.chromium.org/13201/diff/206/10 File src/codegen-ia32.cc (right): http://codereview.chromium.org/13201/diff/206/10#newcode3931 Line 3931: On 2008/12/05 17:29:57, Kevin Millikin wrote: > Take this code out of this change list. I actually think we want to eventually > get rid of the current handling of control, where we possibly get a value in > cc_reg, and possibly on top of the frame, and possibly an unconditional jump and > no frame at all; and then in any case we sometimes have jumps to the control > targets and sometimes not. > > The one rare case where we can currently get unconditional control flow is > slightly unsafe (there are recursive calls to Visit on expressions that don't > properly handle this case), and we should probably not add more (yet). > > A better way is: if there is a pair of labels, we want control flow and we > always get control flow; if there is no pair of labels we do not want control > flow and we never get control flow. > > But that's a big change we should leave for now. Done. http://codereview.chromium.org/13201/diff/206/8 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/13201/diff/206/8#newcode611 Line 611: Unuse(dropped.reg()); On 2008/12/05 17:29:57, Kevin Millikin wrote: > popped Done. http://codereview.chromium.org/13201/diff/206/8#newcode615 Line 615: ASSERT(pop_needed || stack_pointer_ < elements_.length()); On 2008/12/05 17:29:57, Kevin Millikin wrote: > This assert is really checking that the stack pointer points into the virtual > frame on entry to this function. That should always be true, but we should > probably only worry about asserting it where it might go wrong (eg, when > incrementing stack pointer). Done. http://codereview.chromium.org/13201/diff/206/8#newcode616 Line 616: if (pop_needed) --stack_pointer_; On 2008/12/05 17:29:57, Kevin Millikin wrote: > To match the rest of the code base, this should be stack_pointer_-- (though I > don't like it). I also prefer braces and a separate line for the body. Done. http://codereview.chromium.org/13201/diff/206/8#newcode618 Line 618: switch(popped.type()) Oops - no switch http://codereview.chromium.org/13201/diff/206/8#newcode638 Line 638: UNREACHABLE(); On 2008/12/05 17:29:57, Kevin Millikin wrote: > You should be able to restructure this so it doesn't need UNREACHABLE at the end > (ie, that an assert higher up will fail that will give more info about what went > wrong). > > Result VirtualFrame::Pop() { > > > FrameElement popped = elements_.RemoveLast(); > > > bool pop_needed = (stack_pointer_ == elements_.length()); > > > if (popped.is_memory()) { > > > ASSERT(pop_needed); > > > Register temp = cgen_->allocator()->Allocate(); > > ASSERT(!temp.is(no_reg)); > stack_pointer_--; > > > __ pop(temp); > > > return Result(temp); > > > } else { > > > ASSERT(popped.is_register() || popped.is_constant()); > > > if (pop_needed) { > > > stack_pointer_--; > > > __ add(Operand(esp), Immediate(kPointerSize)); > > > } > > > if (popped.is_register()) { > > > frame_registers_.Unuse(popped.reg()); > > > return Result(popped.reg()); > > > } else { > > > return Result(popped.handle()); > > > } > > > } > > } Done. http://codereview.chromium.org/13201/diff/206/9 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/13201/diff/206/9#newcode39 Line 39: // A Result can be a register or a constant (immediate). On 2008/12/05 17:29:57, Kevin Millikin wrote: > Not immediate. It can be constant, in the sense of "known at compile time", but > it is not necessarily immediate in the sense of "can be an immediate operand in > the processor's instruction set". Done. http://codereview.chromium.org/13201/diff/206/9#newcode43 Line 43: Result(Register reg) : type_(REGISTER) { On 2008/12/05 17:29:57, Kevin Millikin wrote: > Make single argument constructors "explicit". Done. http://codereview.chromium.org/13201/diff/206/9#newcode52 Line 52: bool is_register() const { return type() == REGISTER; } On 2008/12/05 17:29:57, Kevin Millikin wrote: > We may want to consider (a) incrementing the registers reference count when the > result is constructed and decrementing when destroyed, or at least (b) asserting > that it has been decremented through this reference when destroyed (requiring an > unuse method on the reference). Done. http://codereview.chromium.org/13201/diff/206/9#newcode302 Line 302: // or a memory location. On 2008/12/05 17:29:57, Kevin Millikin wrote: > Not a memory location (or at least, not yet). Done. http://codereview.chromium.org/13201 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
