OK, I'll take another look. On Thu, Feb 26, 2009 at 2:18 PM, <[email protected]> wrote:
> > http://codereview.chromium.org/21538/diff/4001/4003 > File src/codegen-ia32.cc (right): > > http://codereview.chromium.org/21538/diff/4001/4003#newcode692 > Line 692: // is inlined or should be dealt with in the stub. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> ...or is unneeded. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode693 > Line 693: // GenericBinaryFlags is encoded by XStub in one bit, so only > the > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> I don't understand this comment. In this context, I don't know what >> > XStub is. > >> It's also strange to talk about "passing" the flag to the stub. Isn't >> > the code > >> for the stub parameterized (at code generation time) over the flag? >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode699 > Line 699: SMI_CODE_NOT_SMI > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Needs a better name. The other two tell what they are "the smi code >> > is in the > >> stub" and "the smi code is inlined". Try "NO_SMI_CODE" or something. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode762 > Line 762: // The fastest path is implemented inline. A small deferred > code chunk > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> The sentence starting with "A small..." is confusing. Small relative >> > to what? > >> It could end up rather large, but I don't think we actually care too >> > much here, > >> either. Registers seems overly specific (are they results? they >> > can't be > >> constants?) and vague (what registers?) at the same time. >> > > I would skip all that and just say that there is deferred code that >> > calls the > >> appropriate binary op stub. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode808 > Line 808: > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Extra blank line? >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode837 > Line 837: bool left_const_smi = left.is_constant() && > left.handle()->IsSmi(); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> I would change all the names of these to sound boolean-y. >> > left_is_const_smi, or > >> even just left_is_smi seems better. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode850 > Line 850: // Set flag so that all smi checks are avoided. We know they > will fail. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> It's not that they will fail (one or the other could be a smi), it's >> > that they > >> will not be needed? >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode853 > Line 853: if (right_const_smi) { > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Can't you avoid the nested if? It makes the logic seem more >> > complicated to me. > > } else if (right_const_smi) { >> // generate code, return >> } else if (left_const_smi) { >> // generate coe, return >> } >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode866 > Line 866: // Call the stub and push the result to the stack. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Change this comment so it doesn't mention "stack". >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode899 > Line 899: if (answer != 0 || left >= 0 && right >= 0) { > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Even though precedence will sort this out, parenthesize it. >> > > Also needs some comment about what you're testing for here and why. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode966 > Line 966: // Create a new deferred code for the slow-case part. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Seems like it's not indented. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode970 > Line 970: // Generate the inline part of the code. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> ... including jumps (branches?) to the deferred code. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode1228 > Line 1228: if (!reversed) { > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Change this to if (reversed) and flip the then and else arms. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode1242 > Line 1242: __ mov(answer.reg(), Immediate(value)); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> __ Set? >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode1302 > Line 1302: __ mov(answer.reg(), Operand(operand->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Use the register-register encoding of mov. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode1337 > Line 1337: __ mov(answer.reg(), Operand(operand->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Use the register-register encoding of mov. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode1360 > Line 1360: if (!reversed) { > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Change to if (reversed). >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5461 > Line 5461: // Answer is used to compute the answer, leaving left and > right unchanged. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> This is a strange comment. Left and right are changed by being >> > invalidated. > >> They have already been changed by being possibly moved to registers. >> > And they > >> will be changed if they get spilled by the stub call. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5468 > Line 5468: __ mov(answer.reg(), Operand(left->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Use the register-register encoding of mov. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5475 > Line 5475: __ mov(answer.reg(), Operand(left->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Ditto. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5504 > Line 5504: __ mov(answer.reg(), Operand(left->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Ditto. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5512 > Line 5512: case Token::DIV: > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Needs a "// Fall through." comment. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5515 > Line 5515: // from left and right. The Result answer should be changed > to eax. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Why do they need to be disjoint from left and right (they are not >> > currently)? > >> To preserve left and right for the stub call? I would say it more >> > directly: > >> "Div and mod use the eax and edx registers, and answer must be eax. >> > Left and > >> right must be preserved for the stub call. If either of them are in >> > eax or edx, > >> we move them to another register." >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5553 > Line 5553: // reg_eax is valid, and neither left or right is in eax. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> "nor" :) >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5606 > Line 5606: // Divide edx:eax by ebx. > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Change this comment so it doesn't mention ebx. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5638 > Line 5638: __ test(reg_edx.reg(), Operand(reg_edx.reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> You have a mix of "edx"-style and "reg_edx.reg()" style. Since we've >> > jiggered > >> reg_edx to really be edx, I think it's OK to stick to the former. In >> > any case, > >> be consistent. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5675 > Line 5675: __ mov(left->reg(), Operand(right->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Use the register-register encoding of mov. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5679 > Line 5679: __ mov(answer.reg(), Operand(right->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Ditto. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5684 > Line 5684: __ mov(reg_ecx.reg(), Operand(right->reg())); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Ditto. >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4003#newcode5738 > Line 5738: JumpTarget result_ok(generator()); > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> It seems like the two frames reaching result_ok will be identical, so >> > this is > >> safe to make a raw Label? >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4002 > File src/codegen-ia32.h (right): > > http://codereview.chromium.org/21538/diff/4001/4002#newcode486 > Line 486: // Construct runtime code to perform a binary operation on > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> "Emit code to..." >> > > Done. > > http://codereview.chromium.org/21538/diff/4001/4002#newcode495 > Line 495: // Construct runtime code to perform a binary operation on > On 2009/02/25 21:28:58, Kevin Millikin wrote: > >> Same. >> > > Done. > > > http://codereview.chromium.org/21538 > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
