http://codereview.chromium.org/545007/diff/5033/6018 File src/frame-element.h (right):
http://codereview.chromium.org/545007/diff/5033/6018#newcode56 src/frame-element.h:56: NumberInfo::Type number_info() { For functions like this, where every case returns, I prefer: if (!is_constant) return NumberInfoField::decode(value_); Handle<Object> value = handle(); if (value->IsSmi()) return NumberInfo::kSmi; if (value->IsHeapNumber()) return NumberInfo::kHeapNumber; return NumberInfo::kUnknown; You might also consider that this function is big enough that we don't want to inline it. http://codereview.chromium.org/545007/diff/5033/6018#newcode210 src/frame-element.h:210: FrameElement(Type type, Register reg, SyncFlag is_synced, NumberInfo::Type info) { Long line. http://codereview.chromium.org/545007/diff/5033/6018#newcode253 src/frame-element.h:253: class DataField: public BitField<uint32_t, 8, 32 - 9> {}; I think you've got 24 bits for this field (32 - 8). http://codereview.chromium.org/545007/diff/5033/6013 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/545007/diff/5033/6013#newcode709 src/ia32/codegen-ia32.cc:709: if (frame_->IsNumberAt(0)) { I think results should have number_info too. If they don't then we lose info about values as soon as they leave the frame, and there's no way to associate info with values that aren't on (or never will be on) the frame. The common code here is: Result value = frame_->Pop(); value.ToRegister(); if(value.IsNumber()) { ... } else { ... } You might also consider emitting debug code that verifies that we always have a number at runtime in the only number case. http://codereview.chromium.org/545007/diff/5033/6013#newcode790 src/ia32/codegen-ia32.cc:790: static void LoadSse2Operands(MacroAssembler* masm); I like LoadSSE2Operands better. http://codereview.chromium.org/545007/diff/5033/6014 File src/ia32/codegen-ia32.h (right): http://codereview.chromium.org/545007/diff/5033/6014#newcode715 src/ia32/codegen-ia32.h:715: // Minor key encoding in 16 bits NFRASOOOOOOOOOMM. OK, and just delete the comment about the previous minor key encoding. http://codereview.chromium.org/545007/diff/5033/6015 File src/ia32/virtual-frame-ia32.cc (right): http://codereview.chromium.org/545007/diff/5033/6015#newcode191 src/ia32/virtual-frame-ia32.cc:191: NumberInfo::kUnknownType); // Forget number info. Long line. http://codereview.chromium.org/545007/diff/5033/6015#newcode224 src/ia32/virtual-frame-ia32.cc:224: elements_[i].set_number_info(NumberInfo::kUnknownType); // Forget number info. Long line. http://codereview.chromium.org/545007/diff/5033/6016 File src/ia32/virtual-frame-ia32.h (right): http://codereview.chromium.org/545007/diff/5033/6016#newcode399 src/ia32/virtual-frame-ia32.h:399: NumberInfo::Type GetNumberInfoAt(int index); We don't usually provide an interface for clients to inspect elements of the frame. The biggest reason is that a lot of things (like simply binding a label) can change the frame's state and invalidate client assumptions. http://codereview.chromium.org/545007/diff/5033/6017 File src/jump-target.cc (right): http://codereview.chromium.org/545007/diff/5033/6017#newcode105 src/jump-target.cc:105: FrameElement* other = &reaching_frames_[j]->elements_[i]; I think this is safe but it seems fishy. FrameElement::Combine returns a pointer to one of its arguments. Binding a label should not change what we know about the frames coming into the label. http://codereview.chromium.org/545007/diff/5033/6017#newcode136 src/jump-target.cc:136: entry_frame_->elements_.Add(FrameElement::MemoryElement(NumberInfo::kUninitializedType)); Long line. It seems strange that after this loop, the NULL elements are memory frame elements with Unknown type if they are arguments or local variables, and Uninitialized type if they are in the expression stack. You always reset them anyway, but this seems like a potential bug down the line. http://codereview.chromium.org/545007/diff/5033/6017#newcode171 src/jump-target.cc:171: // If the value is synced on all frames, put it in memory. This You might consider asserting that the type is not uninitialized here. http://codereview.chromium.org/545007/diff/5033/6017#newcode199 src/jump-target.cc:199: FrameElement::NOT_SYNCED, NumberInfo::kUninitializedType); Long line. http://codereview.chromium.org/545007/diff/5033/6017#newcode209 src/jump-target.cc:209: if (direction_ == BIDIRECTIONAL) { I'd get rid of this extra loop. You can check the frame's direction when you combine elements in the very first loop, and also when you set the info for reallocated elements. http://codereview.chromium.org/545007/diff/5033/6012 File src/numberinfo.h (right): http://codereview.chromium.org/545007/diff/5033/6012#newcode1 src/numberinfo.h:1: // Copyright 2010 the V8 project authors. All rights reserved. Can you rename this file to number-info.h? http://codereview.chromium.org/545007/diff/5033/6012#newcode37 src/numberinfo.h:37: kUnknownType = 0, I prefer kUnknown, kSmi, kHeapNumber, etc. "Type" seems redundant. http://codereview.chromium.org/545007/diff/5033/6012#newcode44 src/numberinfo.h:44: static Type Combine(Type a, Type b) { As we discussed, you could simplify this a bit more. http://codereview.chromium.org/545007/diff/5033/6019 File src/virtual-frame.cc (right): http://codereview.chromium.org/545007/diff/5033/6019#newcode102 src/virtual-frame.cc:102: elements_.Add(FrameElement::MemoryElement(NumberInfo::kUninitializedType)); You mean kUnknown? http://codereview.chromium.org/545007/diff/5033/6019#newcode307 src/virtual-frame.cc:307: original.number_info()); This can't be right, you are overwriting the original. I think you have to have Unknown number info here (unless you put number info on Results). http://codereview.chromium.org/545007/diff/5033/6019#newcode333 src/virtual-frame.cc:333: FrameElement::NOT_SYNCED, info); The line breaks here look weird. http://codereview.chromium.org/545007
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
