Experimental: initial simple support for registers in the virtual...

Experimental: initial simple support for registers in the virtual
frame.

It's impossible to do anything interesting without a register file
abstraction to track live registers and merge code for frames
containing registers.

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

[William Hesse, 2008-11-19 15:05:36]

LGTM, with comments.

http://codereview.chromium.org/11472/diff/1/3
File v8/src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/11472/diff/1/3#newcode72
Line 72: bool is_constant() const { return type() == CONSTANT; }
If is_spilled() is only true when type is MEMORY, then
spilling a constant by marking it non-dirty won't be enough - we will have to
forget the fact that it is a constant.
But maybe that is what you mean by spilling - it is needed for
merging, but not for branching or function calls.
Will we have different terms for "everthing non-dirty, all registers free" and
"everything non-dirty, no info at all"?

http://codereview.chromium.org/11472/diff/1/3#newcode100
Line 100: Type type() const { return static_cast<Type>(type_ & kTypeMask); }
Will code constructors only be able to access the type of "Result", to make
decisions on?  That makes sense, I guess.

http://codereview.chromium.org/11472/diff/1/3#newcode286
Line 286: void SpillElementAt(int index);
OK, this answers my question about different terms for spill and sync.

[iposva, 2008-11-20 05:49:35]

There are a couple of things that I would like you to address before submitting
this. Thus I am overriding Bill's positive feedback: Does not yet look good to
me.

-Ivan

http://codereview.chromium.org/11472/diff/1/2
File v8/src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/11472/diff/1/2#newcode102
Line 102: void VirtualFrame::SyncElementAt(int index) {
Comment please.

http://codereview.chromium.org/11472/diff/1/2#newcode119
Line 119: ASSERT(index == stack_pointer_ - 1);
I would expect that elements above the stack pointer are also automatically
marked as dirty. They have to be, no?

That would simplify your logic here as well:
if (element.dirty()) {
  if (index <= stackpointer) {
    ...
  } else {
    ASSERT(index == stack_pointer_ -1 );
    ...
  }
}

I also find the math in the ASSERT rather strange: index is greater than
stack_pointer_, but then you assert that index is equal to stack_pointer minus
one. This seems like a static assertion failure to me. Are you sure this code
has been exercised?

http://codereview.chromium.org/11472/diff/1/2#newcode125
Line 125: stack_pointer_++;
The stack_pointer_ update should be factored out of the if statement.

http://codereview.chromium.org/11472/diff/1/2#newcode132
Line 132: void VirtualFrame::SpillElementAt(int index) {
Comment please.

Especially since the distinction between synching and spilling an element is
relatively small but important.

http://codereview.chromium.org/11472/diff/1/3
File v8/src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/11472/diff/1/3#newcode72
Line 72: bool is_constant() const { return type() == CONSTANT; }
Should there be an is_synched() accessor to make the distinction clearer to the
reader. I am not sure though if it would be useful in the code.

http://codereview.chromium.org/11472/diff/1/3#newcode84
Line 84: private:
C++ style guide suggests this order:
- Typedefs and Enums
- Constants
- Constructors
- Destructor
- Methods, including static methods
- Data Members, including static data members

http://codereview.chromium.org/11472/diff/1/3#newcode85
Line 85: static const int kDirtyBit = 1 << 8;
As I said in my other comments. You probably really want to use the BitField
template here.

http://codereview.chromium.org/11472/diff/1/3#newcode279
Line 279: // frame for all the elements below this one (at least).
Not just the space should be allocated, but all the elements should be on the
stack at least at GC points.

[Kevin Millikin (Chromium), 2008-11-20 08:28:03]

http://codereview.chromium.org/11472/diff/1/2
File v8/src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/11472/diff/1/2#newcode119
Line 119: ASSERT(index == stack_pointer_ - 1);
On 2008/11/20 05:49:35, iposva wrote:
> I would expect that elements above the stack pointer are also automatically
> marked as dirty. They have to be, no?
> 
> That would simplify your logic here as well:
> if (element.dirty()) {
>   if (index <= stackpointer) {
>     ...
>   } else {
>     ASSERT(index == stack_pointer_ -1 );
>     ...
>   }
> }
> 
> I also find the math in the ASSERT rather strange: index is greater than
> stack_pointer_, but then you assert that index is equal to stack_pointer minus
> one. This seems like a static assertion failure to me. Are you sure this code
> has been exercised?

Yes: elements above the stack pointer are always dirty, so your simplified code
is better.

Yes: the assertion is just wrong.  I meant "+ 1" or "- 1" on the other side or
something.

http://codereview.chromium.org/11472/diff/1/2#newcode125
Line 125: stack_pointer_++;
On 2008/11/20 05:49:35, iposva wrote:
> The stack_pointer_ update should be factored out of the if statement.

OK.

http://codereview.chromium.org/11472/diff/1/3
File v8/src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/11472/diff/1/3#newcode72
Line 72: bool is_constant() const { return type() == CONSTANT; }
On 2008/11/20 05:49:35, iposva wrote:
> Should there be an is_synched() accessor to make the distinction clearer to
the
> reader. I am not sure though if it would be useful in the code.

Maybe it's best to rename is_spilled to something more obvious.  is_in_memory
seems accurate but I don't like reading it.

http://codereview.chromium.org/11472/diff/1/3#newcode84
Line 84: private:
On 2008/11/20 05:49:35, iposva wrote:
> C++ style guide suggests this order:
> - Typedefs and Enums
> - Constants
> - Constructors
> - Destructor
> - Methods, including static methods
> - Data Members, including static data members
> 

It seems like we often have data members first in our code base (at least
private ones).  But since this is new code, there is no reason not to stick to
the style guide.  I'll change it.

http://codereview.chromium.org/11472/diff/1/3#newcode100
Line 100: Type type() const { return static_cast<Type>(type_ & kTypeMask); }
On 2008/11/19 15:05:36, William Hesse wrote:
> Will code constructors only be able to access the type of "Result", to make
> decisions on?  That makes sense, I guess.

I decided not to expose the type (in fact, the enum should probably be private
too).  Internally, we might have a lot of different "types" for administrative
purposes (eg, constant vs. source constant) that the clients of the frame don't
care about.

The place where types might be better than predicates is if clients want to
write a switch instead of nested if-then-else.  But that code is brittle to
addition of types anyway....

http://codereview.chromium.org/11472/diff/1/3#newcode279
Line 279: // frame for all the elements below this one (at least).
On 2008/11/20 05:49:35, iposva wrote:
> Not just the space should be allocated, but all the elements should be on the
> stack at least at GC points.

I agree that "sync" vs. "spill" is fuzzy.  The point is that we sometimes want
to allocate space in the frame to spill a value (in case we later need to spill
it), without actually spilling it (eg, without forgetting that it is a constant
or in a particular register).

But, as you say, we have to put something safe for GC in that slot so it makes
sense to put the real value there and avoid a move later in the event of a spill
before the frame element is changed again.

Thus "sync"---to make the value in memory agree with the frame but not change
the frame value.

It will likely only be used for allocating space for a spill, so changing the
name probably removes confusion (I just can't think of a better name right now).