Begin counting references to registers, both in the frame and out....

Begin counting references to registers, both in the frame and out.
Allocation/deallocation is explicit.  We still almost always spill
everything and then use registers directly (without allocating them).

None of this has been ported to the ARM.

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

[William Hesse, 2008-11-24 13:44:51]

Figure out where the register allocator's increments and decrements happen when
register elements are added to and dropped from the frame.  We are currently
thinking in the constructor and destructor of register elements, but this
requires a back pointer in the elements.  The frame copy code, push, pop, and
drop code all need to agree on this.

http://codereview.chromium.org/11396/diff/1/9
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/11396/diff/1/9#newcode290
Line 290: allocator_ = NULL;
// Insert a comment for dumb people like me: allocator_ points to a
stack-allocated local.

http://codereview.chromium.org/11396/diff/1/6
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/11396/diff/1/6#newcode67
Line 67: frame_registers_(original->frame_registers_) {
If the register_elements have pointers to the frame, or to the frame's register
allocator, so that their destructors can decrement the allocator counts, then
these pointers need to be redirected to the new copy when the list is copied. 
Actually, the problem is in the loop following, that copies the elements.

http://codereview.chromium.org/11396/diff/1/6#newcode156
Line 156: // register whose internal reference count matches its internal
reference
matches its external reference count?

http://codereview.chromium.org/11396/diff/1/6#newcode177
Line 177: if (elements_[i].is_register() &
&& not &, right?

http://codereview.chromium.org/11396/diff/1/6#newcode202
Line 202: // above the stack pointer.
UNclear about "We can only..."
Comment or assert precondition:
index <= stack pointer + 1

http://codereview.chromium.org/11396/diff/1/6#newcode391
Line 391: void VirtualFrame::Drop(int count) {
Drop needs to update the register allocator.  We will
do this in the destructor of the frame elements.

http://codereview.chromium.org/11396/diff/1/6#newcode426
Line 426: void VirtualFrame::EmitPush(Register reg) {
Should this ever be used with regs?  Assert that it is not?

http://codereview.chromium.org/11396/diff/1/6#newcode451
Line 451: Use(reg);
Change Use(reg) to be in the constructor of the 
FrameElement(reg, this) and Unuse(reg) to be in its destructor.

[iposva, 2008-11-25 07:11:01]

First set of comments. I have not looked in detail yet at the allocator.

I guess this still needs more work, correct?

-Ivan

http://codereview.chromium.org/11396/diff/1/6
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/11396/diff/1/6#newcode165
Line 165: (count < min_count || min_count == -1)) {
By starting the min_count with max integer, you can avoid the complicated
condition.

http://codereview.chromium.org/11396/diff/1/6#newcode176
Line 176: for (; i < stack_pointer_ && min_count > 0; i++) {
Personal taste: This is really a while loop, and not a counted for loop.

http://codereview.chromium.org/11396/diff/1/6#newcode184
Line 184: for (; i < elements_.length() && min_count > 0; i++) {
These two loops should be rolled into one. They essentially do the same work and
roll the check whether to sync or not into the loop condition. It is very hard
to understand what the difference of the two loops is supposed to mean
especially in the absence of comments. I would suggest restructuring it as
follows. This also entirely avoids the style comment I had above.

for (int i = 0; min_count > 0; i++) {
  ASSERT(i < elements.length());
  if (elements_[i].is_register() && elements[i]...) {
    // Found one instance of the best_register being used. Spill it.
    SpillElementAt(i);
    min_count--;
  } else {
    if (i > stack_pointer_) {  <- I have fixed the off by one error here.
      // Make sure to materialize elements on the virtual stack in memory. We
rely
      // on the fact that all elements below a value being written to memory
have
      // been flushed to the physical stack.
      SyncElementAt(i);
    }
  }
}

http://codereview.chromium.org/11396/diff/1/6#newcode190
Line 190: SyncElementAt(i);
It looks to me that there is an off by one error in these two dependent loops.
The first loop will exit if (i==stack_pointer_) and the code here will call
SyncElementAt(i). The condition within SyncElementAt is i<=stack_pointer_, which
means that either the stack_pointer_ needs a better abstraction and/or that the
two nested loops are too confusing.

http://codereview.chromium.org/11396/diff/1/6#newcode391
Line 391: void VirtualFrame::Drop(int count) {
On 2008/11/24 13:44:51, William Hesse wrote:
> Drop needs to update the register allocator.  We will
> do this in the destructor of the frame elements.

RemoveLast does not delete the elements and thus it does not call the destructor
for the frame elements.

http://codereview.chromium.org/11396/diff/1/6#newcode428
Line 428: elements_.Add(FrameElement());
I find this style very confusing. I did not notice this earlier on, but this
looks like a local method call, but it actually constructs a temporary object,
which will be copied withing the Add method.

http://codereview.chromium.org/11396/diff/1/6#newcode451
Line 451: Use(reg);
On 2008/11/24 13:44:51, William Hesse wrote:
> Change Use(reg) to be in the constructor of the 
> FrameElement(reg, this) and Unuse(reg) to be in its destructor.
> 

As I pointed out above, you cannot rely on the destructors for the BASE_EMBEDDED
types to be called when you "expect" them to be called. Especially when it is
added to a List.

Also you do not necessarily want to Use within the constructor even if the above
problem were not there because you loose control of the allocation. An example
of this is the function entry code that needs to populate an initial state.

http://codereview.chromium.org/11396/diff/1/6#newcode452
Line 452: elements_.Add(FrameElement(reg));
Same comment as above about the constructor call looking like a function call
here but "creating" an anonymous object.

[Kevin Millikin (Chromium), 2008-11-26 12:49:01]

Thanks for the comments.  New patch set uploaded.