OK LGTM Great
On Tue, Nov 18, 2008 at 8:12 PM, <[EMAIL PROTECTED]> wrote: > > http://codereview.chromium.org/11232/diff/201/13 > File src/codegen-ia32.cc (left): > > http://codereview.chromium.org/11232/diff/201/13#oldcode203 > Line 203: // Save the arguments object pointer, if any. > On 2008/11/18 16:37:34, William Hesse wrote: > >> What is this change? >> > > As far as I can tell, the only reason to save ecx here is because it > will be clobbered by SlotOperand and RecordWrite. All the gymnastics > with saving ecx will go away as soon as we can support registers in the > virtual frame, anyway. > > http://codereview.chromium.org/11232/diff/201/10 > File src/virtual-frame-ia32.cc (right): > > http://codereview.chromium.org/11232/diff/201/10#newcode49 > Line 49: frame_pointer_(kIllegalIndex) { > On 2008/11/18 16:37:34, William Hesse wrote: > >> In this case, is IllegalIndex actually the accurate value of >> > frame_pointer, not > >> a sign of illegality? >> Perhaps illegalIndex should not be -1, but should be a less >> likely negative value. >> > > We don't know what the frame pointer is until we set it in Enter(). > > http://codereview.chromium.org/11232/diff/201/10#newcode77 > Line 77: ASSERT(stack_pointer_ == elements_.length() - 1); > On 2008/11/18 16:37:34, William Hesse wrote: > >> Isn't this too strong a precondition for a useful Adjust()? Won't you >> > want to > >> use Adjust on more complex frames? >> > > I think it's right. The intent of "Adjust" is that it doesn't generate > code but only fixes up the frame to reflect code already (or about to > be) generated. > > If the sp is not at the top of the virtual frame when adjust is called, > the it's not safe to have someone else push on the stack. > > http://codereview.chromium.org/11232/diff/201/10#newcode109 > Line 109: __ mov(Operand(ebp, fp_relative(i)), > Immediate(elements_[i].handle())); > On 2008/11/18 16:37:34, William Hesse wrote: > >> Is the splitting into two 16-bit quantities inside mov? >> > > There is no splitting here (yet). The only constants right now are not > from source code. > > http://codereview.chromium.org/11232/diff/201/11 > File src/virtual-frame-ia32.h (right): > > http://codereview.chromium.org/11232/diff/201/11#newcode52 > Line 52: Type type() const { return static_cast<Type>(type_ & ~0x1); } > On 2008/11/18 16:37:34, William Hesse wrote: > >> Name the dirty flag mask. Unless you are also storing SMIs in this >> > word, I > >> would use a higher bit for the flag, and store the types in the low >> > bits, rather > >> than using only even type IDs. >> > > OK. > > http://codereview.chromium.org/11232/diff/201/11#newcode66 > Line 66: Handle<Object> handle() const { return > Handle<Object>(data_.handle_); } > On 2008/11/18 16:37:34, William Hesse wrote: > >> Put asserts here to assert it is a type that has handle data? >> > > OK. > > http://codereview.chromium.org/11232/diff/201/11#newcode254 > Line 254: void PrepareCall(int count); > On 2008/11/18 16:37:34, William Hesse wrote: > >> PrepareForCall? >> >> > OK. > > > http://codereview.chromium.org/11232 > -- We can IMAGINE what is not --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
