I like this approach! It's very simple. I've got some comments below.
http://codereview.chromium.org/7976024/diff/5001/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7976024/diff/5001/src/heap.cc#newcode2246 src/heap.cc:2246: Smi::FromInt(-5), I guess it's useful to give the hidden oddballs distinct toNumber values, but then you should either reorder their allocation so the numbers are consecutive or renumber them. http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc File src/ia32/deoptimizer-ia32.cc (right): http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode444 src/ia32/deoptimizer-ia32.cc:444: int frame_pointer = output_[0]->GetRegister(ebp.code()); Don't put this here (i.e., don't set ebp register and then patch it up just later). It's too easy to miss what's going on. Please just set it to the value you want in the first place, about 8 lines above. http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode502 src/ia32/deoptimizer-ia32.cc:502: // If the optimized frame had alignment padding, adjust the frame pointer I also don't like this here. First, it breaks up the comment about setting the top address so it's now far from the code that sets the top address. Worse, I think it's a bug waiting to happen to just spoof the value in the input ebp register. I'd rather leave the correct value there and adjust for padding in a couple of places. http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode514 src/ia32/deoptimizer-ia32.cc:514: input_->GetRegister(ebp.code()) - (2 * kPointerSize) - height_in_bytes; Here: input_->GetRegister(ebp.code()) - (2 * kPointerSize) - height_in_bytes + (has_alignment_padding_ * kPointerSize) http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode565 src/ia32/deoptimizer-ia32.cc:565: ASSERT(!is_bottommost || input_->GetRegister(ebp.code()) == fp_value); Here: ASSERT(!is_bottommost || input_->GetRegister(ebp.code()) + has_alignment_padding * kPointerSize == fp_value); http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode160 src/ia32/lithium-codegen-ia32.cc:160: __ mov(ebx, esp); Can you test(esp, 4)? http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode161 src/ia32/lithium-codegen-ia32.cc:161: __ and_(ebx, Immediate(0x4)); Is this better (kDoubleSize - 1) or (kDoubleSize >> 1) or some other named constant? http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode163 src/ia32/lithium-codegen-ia32.cc:163: __ mov(ebx, esp); The last move after the loop might be slightly more obvious if you make ebx be the address you're copying to, rather than from. http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode165 src/ia32/lithium-codegen-ia32.cc:165: __ mov(ecx, Immediate(scope()->num_parameters() + 2)); Comment should say 2 is receiver + return address. http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode2045 src/ia32/lithium-codegen-ia32.cc:2045: __ cmp(Operand(ebp, (GetParameterCount() + 3) * kPointerSize), 3 ~ caller's fp, return address, receiver. http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode2049 src/ia32/lithium-codegen-ia32.cc:2049: __ pop(ebp); Slightly more compact is: __ mov(esp, ebp); __ pop(ebp); __ cmp(Operand(esp, (param_count + 2) * kPointerSize), marker); __ j(not_equal, &aligned); __ Ret(parameter_count + 2); __ bind(&aligned); __ Ret(parameter_count + 1); http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.h File src/ia32/lithium-codegen-ia32.h (right): http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.h#newcode138 src/ia32/lithium-codegen-ia32.h:138: void set_dynamic_frame_alignment(bool value) { I don't think you need a private setter that has one call site. http://codereview.chromium.org/7976024/diff/5001/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7976024/diff/5001/src/objects.h#newcode6581 src/objects.h:6581: static const byte kFrameAlignmentMarker = 6; You never use this constant that I can see. Can you just reuse kOther? I'm not sure it's critical to have as many tags as oddballs. http://codereview.chromium.org/7976024/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
