Lots of comments, mostly style.
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. ...or is unneeded. http://codereview.chromium.org/21538/diff/4001/4003#newcode693 Line 693: // GenericBinaryFlags is encoded by XStub in one bit, so only the 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? http://codereview.chromium.org/21538/diff/4001/4003#newcode699 Line 699: SMI_CODE_NOT_SMI 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. http://codereview.chromium.org/21538/diff/4001/4003#newcode762 Line 762: // The fastest path is implemented inline. A small deferred code chunk 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. http://codereview.chromium.org/21538/diff/4001/4003#newcode808 Line 808: Extra blank line? http://codereview.chromium.org/21538/diff/4001/4003#newcode837 Line 837: bool left_const_smi = left.is_constant() && left.handle()->IsSmi(); I would change all the names of these to sound boolean-y. left_is_const_smi, or even just left_is_smi seems better. 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. It's not that they will fail (one or the other could be a smi), it's that they will not be needed? http://codereview.chromium.org/21538/diff/4001/4003#newcode853 Line 853: if (right_const_smi) { 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 } http://codereview.chromium.org/21538/diff/4001/4003#newcode866 Line 866: // Call the stub and push the result to the stack. Change this comment so it doesn't mention "stack". http://codereview.chromium.org/21538/diff/4001/4003#newcode899 Line 899: if (answer != 0 || left >= 0 && right >= 0) { Even though precedence will sort this out, parenthesize it. Also needs some comment about what you're testing for here and why. http://codereview.chromium.org/21538/diff/4001/4003#newcode966 Line 966: // Create a new deferred code for the slow-case part. Seems like it's not indented. http://codereview.chromium.org/21538/diff/4001/4003#newcode970 Line 970: // Generate the inline part of the code. ... including jumps (branches?) to the deferred code. http://codereview.chromium.org/21538/diff/4001/4003#newcode1228 Line 1228: if (!reversed) { Change this to if (reversed) and flip the then and else arms. http://codereview.chromium.org/21538/diff/4001/4003#newcode1242 Line 1242: __ mov(answer.reg(), Immediate(value)); __ Set? http://codereview.chromium.org/21538/diff/4001/4003#newcode1302 Line 1302: __ mov(answer.reg(), Operand(operand->reg())); Use the register-register encoding of mov. http://codereview.chromium.org/21538/diff/4001/4003#newcode1337 Line 1337: __ mov(answer.reg(), Operand(operand->reg())); Use the register-register encoding of mov. http://codereview.chromium.org/21538/diff/4001/4003#newcode1360 Line 1360: if (!reversed) { Change to if (reversed). http://codereview.chromium.org/21538/diff/4001/4003#newcode5461 Line 5461: // Answer is used to compute the answer, leaving left and right unchanged. 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. http://codereview.chromium.org/21538/diff/4001/4003#newcode5468 Line 5468: __ mov(answer.reg(), Operand(left->reg())); Use the register-register encoding of mov. http://codereview.chromium.org/21538/diff/4001/4003#newcode5475 Line 5475: __ mov(answer.reg(), Operand(left->reg())); Ditto. http://codereview.chromium.org/21538/diff/4001/4003#newcode5504 Line 5504: __ mov(answer.reg(), Operand(left->reg())); Ditto. http://codereview.chromium.org/21538/diff/4001/4003#newcode5512 Line 5512: case Token::DIV: Needs a "// Fall through." comment. http://codereview.chromium.org/21538/diff/4001/4003#newcode5515 Line 5515: // from left and right. The Result answer should be changed to eax. 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." http://codereview.chromium.org/21538/diff/4001/4003#newcode5553 Line 5553: // reg_eax is valid, and neither left or right is in eax. "nor" :) http://codereview.chromium.org/21538/diff/4001/4003#newcode5606 Line 5606: // Divide edx:eax by ebx. Change this comment so it doesn't mention ebx. http://codereview.chromium.org/21538/diff/4001/4003#newcode5638 Line 5638: __ test(reg_edx.reg(), Operand(reg_edx.reg())); 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. http://codereview.chromium.org/21538/diff/4001/4003#newcode5675 Line 5675: __ mov(left->reg(), Operand(right->reg())); Use the register-register encoding of mov. http://codereview.chromium.org/21538/diff/4001/4003#newcode5679 Line 5679: __ mov(answer.reg(), Operand(right->reg())); Ditto. http://codereview.chromium.org/21538/diff/4001/4003#newcode5684 Line 5684: __ mov(reg_ecx.reg(), Operand(right->reg())); Ditto. http://codereview.chromium.org/21538/diff/4001/4003#newcode5738 Line 5738: JumpTarget result_ok(generator()); It seems like the two frames reaching result_ok will be identical, so this is safe to make a raw Label? 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 "Emit code to..." http://codereview.chromium.org/21538/diff/4001/4002#newcode495 Line 495: // Construct runtime code to perform a binary operation on Same. http://codereview.chromium.org/21538 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
