Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(639)

Issue 11472: Experimental: initial simple support for registers in the virtual... (Closed)

Created:
12 years, 1 month ago by Kevin Millikin (Chromium)
Modified:
9 years, 7 months ago
Reviewers:
iposva, William Hesse
CC:
v8-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -37 lines) Patch
M v8/src/checks.h View 1 chunk +1 line, -1 line 0 comments Download
M v8/src/virtual-frame-ia32.h View 5 chunks +30 lines, -4 lines 11 comments Download
M v8/src/virtual-frame-ia32.cc View 5 chunks +85 lines, -32 lines 6 comments Download

Messages

Total messages: 4 (0 generated)
Kevin Millikin (Chromium)
12 years, 1 month ago (2008-11-19 13:22:45 UTC) #1
William Hesse
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 ...
12 years, 1 month ago (2008-11-19 15:05:36 UTC) #2
iposva
There are a couple of things that I would like you to address before submitting ...
12 years, 1 month ago (2008-11-20 05:49:35 UTC) #3
Kevin Millikin (Chromium)
12 years, 1 month ago (2008-11-20 08:28:03 UTC) #4
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).

Powered by Google App Engine
This is Rietveld 408576698