OK, some comments addressed. Other changes, to MergeTo, moved to separate change list. No new review needed yet.
http://codereview.chromium.org/13665/diff/1/3 File src/codegen-ia32.cc (right): http://codereview.chromium.org/13665/diff/1/3#newcode1183 Line 1183: int value, On 2008/12/10 01:04:19, iposva wrote: > Generally I would expect the order of parameters for operation values as left to > right. So you will want to change the order passing the reg before the value. Done. http://codereview.chromium.org/13665/diff/1/3#newcode1184 Line 1184: Register arg_1) On 2008/12/09 19:00:50, Kevin Millikin wrote: > There must be a better name. Done. http://codereview.chromium.org/13665/diff/1/3#newcode1212 Line 1212: // TODO On 2008/12/09 19:00:50, Kevin Millikin wrote: > ??? Done. http://codereview.chromium.org/13665/diff/1/3#newcode1223 Line 1223: Result temp = frame_->Pop(); On 2008/12/09 19:00:50, Kevin Millikin wrote: > Must be a better name. It's not really a temp, it's one of the operands of the > comparison. Done. http://codereview.chromium.org/13665/diff/1/3#newcode1225 Line 1225: Register r1 = allocator()->Allocate(); On 2008/12/10 01:04:19, iposva wrote: > I do not understand why you bother loading the constant value into a register in > this case. You are comparing two constants so the outcome should be determined > at compile time and you can branch straight to the appropriate label. > > P.S. r1 is a very unlucky choice for a register variable name. I know that it > does not matter for the IA32 code gen. In a sense you would have wanted to use > temp_reg here, but temp was already used (and Kevin already commented on that > name). Done. http://codereview.chromium.org/13665/diff/1/3#newcode1227 Line 1227: __ mov(r1, Immediate(temp.handle())); // ?? On 2008/12/09 19:00:50, Kevin Millikin wrote: > Use __ Set when the rhs is an immediate. Done. http://codereview.chromium.org/13665/diff/1/3#newcode1229 Line 1229: // so r1 is not double counted, so fine? On 2008/12/09 19:00:50, Kevin Millikin wrote: > I'm not sure what the comment means, but this should be sorted out. My > preference is that we never *explicitly* construct result values outside of the > virtual frame and the result class itself (possibly the register allocator). Done. http://codereview.chromium.org/13665 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
