Remove register counts from VirtualFrame, use register indices instead.

Remove register counts from VirtualFrame, use register indices instead.

Committed: http://code.google.com/p/v8/source/detail?r=1605

[Kevin Millikin (Chromium), 2009-03-25 08:48:18]

This seems like a nice cleanup even if it doesn't make a noticeable performance
difference.

http://codereview.chromium.org/42565/diff/1/2
File src/jump-target.cc (right):

http://codereview.chromium.org/42565/diff/1/2#newcode275
Line 275: // Set the register counts and indices.
Update this comment to go with the code.

http://codereview.chromium.org/42565/diff/1/8
File src/register-allocator.cc (right):

http://codereview.chromium.org/42565/diff/1/8#newcode110
Line 110: if (count(target) == (cgen_->frame()->is_used(target) ? 1 : 0))  {
Don't you want something more like

cgen_->frame()->is_used(target) && count(target) == 1

It seems a lot clearer, since you've already taken care of the case
!is_used(target) (and you wouldn't want to call spill on the frame if the
register isn't used there anyway).

http://codereview.chromium.org/42565/diff/1/6
File src/virtual-frame-arm.h (right):

http://codereview.chromium.org/42565/diff/1/6#newcode80
Line 80: return frame_registers_.count(reg);
You should remove this function and the RegisterFile from the ARM frame class.

http://codereview.chromium.org/42565/diff/1/5
File src/virtual-frame.cc (right):

http://codereview.chromium.org/42565/diff/1/5#newcode196
Line 196: } else if (cgen_->allocator()->count(i) == 0) {
The only call site of this function has already searched for unused registers,
so we don't need to repeat that work here.  I think you should be able to skip
the else if and break out of the loop as soon as you find a register to spill.

http://codereview.chromium.org/42565/diff/1/5#newcode387
Line 387: if (!is_used(value->reg())) {
I like to flip these ifs with elses so the condition is not negated.

http://codereview.chromium.org/42565/diff/1/5#newcode487
Line 487: new_element = CopyElementAt(register_index(reg));
You could consider just adding the copy to elements_ (and the register in the
other branch of the if) rather than assigning to a local and adding later.

[William Hesse, 2009-03-25 09:45:20]

All recommended changes made.

http://codereview.chromium.org/42565/diff/1/2
File src/jump-target.cc (right):

http://codereview.chromium.org/42565/diff/1/2#newcode275
Line 275: // Set the register counts and indices.
On 2009/03/25 08:48:19, Kevin Millikin wrote:
> Update this comment to go with the code.

Done.

http://codereview.chromium.org/42565/diff/1/6
File src/virtual-frame-arm.h (right):

http://codereview.chromium.org/42565/diff/1/6#newcode80
Line 80: return frame_registers_.count(reg);
On 2009/03/25 08:48:19, Kevin Millikin wrote:
> You should remove this function and the RegisterFile from the ARM frame class.

Done.

http://codereview.chromium.org/42565/diff/1/5
File src/virtual-frame.cc (right):

http://codereview.chromium.org/42565/diff/1/5#newcode196
Line 196: } else if (cgen_->allocator()->count(i) == 0) {
On 2009/03/25 08:48:19, Kevin Millikin wrote:
> The only call site of this function has already searched for unused registers,
> so we don't need to repeat that work here.  I think you should be able to skip
> the else if and break out of the loop as soon as you find a register to spill.

Yes, I think so to, but then this function has a precondition.  I'll do it. 
Done.

http://codereview.chromium.org/42565/diff/1/5#newcode387
Line 387: if (!is_used(value->reg())) {
On 2009/03/25 08:48:19, Kevin Millikin wrote:
> I like to flip these ifs with elses so the condition is not negated.

I thought it was better to be able to quickly read about the free-register case,
then worry about the bigger other case.  But I can switch itl
Done.

http://codereview.chromium.org/42565/diff/1/5#newcode487
Line 487: new_element = CopyElementAt(register_index(reg));
On 2009/03/25 08:48:19, Kevin Millikin wrote:
> You could consider just adding the copy to elements_ (and the register in the
> other branch of the if) rather than assigning to a local and adding later.

Done.

[Kevin Millikin (Chromium), 2009-03-25 09:54:11]

Other than the issue mentioned below, it LGTM.

http://codereview.chromium.org/42565/diff/1004/1008
File src/virtual-frame.cc (right):

http://codereview.chromium.org/42565/diff/1004/1008#newcode189
Line 189: // in the CodeGenerator's Allocator must be above 0.
I don't like this comment.  It's not really a true precondition (the function
still works fine otherwise), and if it were a precondition it shouldn't be
buried here in the implementation.

[William Hesse, 2012-05-07 09:35:42]

http://codereview.chromium.org/42565/diff/1004/src/virtual-frame.cc
File src/virtual-frame.cc (right):

http://codereview.chromium.org/42565/diff/1004/src/virtual-frame.cc#newcode189
src/virtual-frame.cc:189: // in the CodeGenerator's Allocator must be above 0.
On 2009/03/25 09:54:11, Kevin Millikin wrote:
> I don't like this comment.  It's not really a true precondition (the function
> still works fine otherwise), and if it were a precondition it shouldn't be
> buried here in the implementation.

Done.