Still needs some work. I think you've (uniformly) swapped the order of the frame elements in Comparison.
I'm also worried that the explicit register shuffling code before calling the compare stub won't scale well. At this site it is only temporary because we will probably specialize the compare stub to its argument registers. In the places where we actually need to shuffle registers, if any, we should consider ways to reuse the same register-to-register move code as for the frame merge. We ought to be able to emit the same code automatically. http://codereview.chromium.org/16450/diff/1/7 File src/assembler-ia32.cc (right): http://codereview.chromium.org/16450/diff/1/7#newcode751 Line 751: dst = src; // No need to copy eax to src, we assume src is eax. More straightforward: EMIT(0x90 | (dst.is(eax) ? src.code() : dst.code())); http://codereview.chromium.org/16450/diff/1/8 File src/codegen-ia32.cc (right): http://codereview.chromium.org/16450/diff/1/8#newcode1193 Line 1193: right_side = frame_->Pop(); I'm confused by this: the rhs is normally on top of the lhs on the stack, so I would expect the rhs to be popped first in the normal case and the lhs to be popped first in the reversed case. http://codereview.chromium.org/16450/diff/1/8#newcode1213 Line 1213: __ cmp(right_side.reg(), Operand(left_side.reg())); Here is where the variable naming/popping order confusion manifests itself. Why is right_side on the left? http://codereview.chromium.org/16450/diff/1/8#newcode1214 Line 1214: // Fall through to |done|. Did we need to change the order we emit the smi and non-smi cases? We've now grown an extra unconditional jump on the fast path. We should instead work hard to make merging work properly with the original basic block order (and leave the original order for now even if its temporarily suboptimal). If you leave it as you've changed it, please update the comments (here we're not falling through to done, we're jumping to it....). http://codereview.chromium.org/16450/diff/1/8#newcode1222 Line 1222: Remove the extra blank line. http://codereview.chromium.org/16450/diff/1/8#newcode4201 Line 4201: frame_->Push(eax); // push the result Get rid of this explicit (and non-counted) reference to eax in non-spilled code. Instead, VirtualFrame::InvokeBuiltin could return a result that can be used to manage the result register resource (you should also get rid of the version of VirtualFrame::Push that takes a register). http://codereview.chromium.org/16450/diff/1/8#newcode4209 Line 4209: __ test(eax, Operand(eax)); Ditto. http://codereview.chromium.org/16450/diff/1/5 File src/jump-target-ia32.cc (right): http://codereview.chromium.org/16450/diff/1/5#newcode178 Line 178: Result::Type arg_1_type = arg_1->type(); These assertions don't scale so well. We should try to come up with a macro for the binding site and a function for the test. That can be a separate change. http://codereview.chromium.org/16450/diff/1/6 File src/jump-target.h (right): http://codereview.chromium.org/16450/diff/1/6#newcode101 Line 101: void Branch(Condition cc, Result* arg_1, Result* arg_2, Hint hint = no_hint); Please call these arguments arg0 and arg1 (zero based indexing like C++ and without the underscore). http://codereview.chromium.org/16450/diff/1/6#newcode107 Line 107: void Bind(Result* arg_1, Result* arg_2); Ditto. http://codereview.chromium.org/16450/diff/1/3 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/16450/diff/1/3#newcode135 Line 135: if (!frame_registers_.is_used(target.code())) return; Use the overloaded version of is_used that takes a Register argument. http://codereview.chromium.org/16450 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
