Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13201:
A first stab at using the top of stack as the... (Closed)
|
Created:
12 years ago by William Hesse Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionA first stab at using the top of stack as the
register it is currently in, or as the constant it is.
Probably doesn't even compile. Take a look.
Committed: http://code.google.com/p/v8/source/detail?r=949
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 25
Patch Set 4 : '' #Patch Set 5 : '' #Messages
Total messages: 4 (0 generated)
If the general direction is right, I will start working on the BinaryComparison to a literal case.
Looks like the right direction, with lots of comments below. Get rid of the use in CodeGenerator before committing. We should talk Monday about where would be a good first place to start using this. One possibility is to get rid of the performance regression caused by missed peephole optimization (namely, assignments whose rhs is a function call). 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: 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. http://codereview.chromium.org/13201/diff/206/10#newcode3934 Line 3934: Object* value = result.handle()->value(); Handle<Object> value = result.handle(); Don't use raw pointers in handle code (all the Factory stuff below). In general, the Factory calls could trigger a GC. http://codereview.chromium.org/13201/diff/206/10#newcode3935 Line 3935: if (value == Factory::null_value()) { value->IsNull() http://codereview.chromium.org/13201/diff/206/10#newcode3938 Line 3938: if (op == Token::EQ_STRICT) { You want else if here (preferred) or return just above, otherwise you are generating dead code in the case that the first jump was taken. http://codereview.chromium.org/13201/diff/206/10#newcode3941 Line 3941: if (value == Factory::undefined()) { value->IsUndefined() http://codereview.chromium.org/13201/diff/206/10#newcode3944 Line 3944: if (value->IsSMI()) { value->IsSmi() http://codereview.chromium.org/13201/diff/206/10#newcode3962 Line 3962: true_target()->Branch(equal); Another reason to remove this code: we have to sort out what to do with the Result (a register not owned by the frame) at the basic blocks. 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()); popped http://codereview.chromium.org/13201/diff/206/8#newcode615 Line 615: ASSERT(pop_needed || stack_pointer_ < elements_.length()); 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). http://codereview.chromium.org/13201/diff/206/8#newcode616 Line 616: if (pop_needed) --stack_pointer_; 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. http://codereview.chromium.org/13201/diff/206/8#newcode638 Line 638: UNREACHABLE(); 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()); } } } 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). 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". http://codereview.chromium.org/13201/diff/206/9#newcode43 Line 43: Result(Register reg) : type_(REGISTER) { Make single argument constructors "explicit". http://codereview.chromium.org/13201/diff/206/9#newcode52 Line 52: bool is_register() const { return type() == REGISTER; } 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). http://codereview.chromium.org/13201/diff/206/9#newcode302 Line 302: // or a memory location. Not a memory location (or at least, not yet).
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.
After fixing the style issues we discussed offline, LGTM. |
