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). http://codereview.chromium.org/11472 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
