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 -~----------~----~----~----~------~----~------~--~---
