This changelist has been copied to a new issue number, because edits were made on a different checked-out copy of V8. Further comments and work are at http://codereview.chromium.org/16481
On 2008/12/29 11:47:42, William Hesse wrote: > 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. > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > More straightforward: > > EMIT(0x90 | (dst.is(eax) ? src.code() : dst.code())); > > Done. > > 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(); > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > 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. > > True. Left and right were consistently flipped through the entire function. > Left has been renamed to right, and right to left, so now they are correct. > > http://codereview.chromium.org/16450/diff/1/8#newcode1213 > Line 1213: __ cmp(right_side.reg(), Operand(left_side.reg())); > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > Here is where the variable naming/popping order confusion manifests itself. > Why > > is right_side on the left? > > Done. > > http://codereview.chromium.org/16450/diff/1/8#newcode1214 > Line 1214: // Fall through to |done|. > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > 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....). > > Changed back, so there is not an unconditional jump on the fast case. > The JumpTarget "done" is bound to the slow case first, so we are probably > suboptimal until we handle merges better. > > http://codereview.chromium.org/16450/diff/1/8#newcode1222 > Line 1222: > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > Remove the extra blank line. > > Done. > > http://codereview.chromium.org/16450/diff/1/8#newcode4201 > Line 4201: frame_->Push(eax); // push the result > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > 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). > > I would like to make this a separate change, to be done immediately. > > http://codereview.chromium.org/16450/diff/1/8#newcode4209 > Line 4209: __ test(eax, Operand(eax)); > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > Ditto. > > To be done in 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); > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > Please call these arguments arg0 and arg1 (zero based indexing like C++ and > > without the underscore). > > Done. > > http://codereview.chromium.org/16450/diff/1/6#newcode107 > Line 107: void Bind(Result* arg_1, Result* arg_2); > On 2008/12/29 09:05:38, Kevin Millikin wrote: > > Ditto. > > Done. http://codereview.chromium.org/16450 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
