Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21538:
Add constant folding and optimization for constant smi operands to... (Closed)
|
Created:
11 years, 10 months ago by William Hesse Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionAdd constant folding and optimization for constant smi operands to
binary operations. Move existing smi optimizations from AST level
to virtual frame level.
Patch Set 1 #
Total comments: 1
Patch Set 2 : '' #
Total comments: 16
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 64
Patch Set 8 : '' #Messages
Total messages: 9 (0 generated)
Please take a preliminary look at these changes. They are not yet debugged or tested, but I could use some feedback.
I think the approach looks right. We should make sure to come up with tests that hit all the new code paths. http://codereview.chromium.org/21538/diff/1/2 File src/codegen-ia32.h (right): http://codereview.chromium.org/21538/diff/1/2#newcode483 Line 483: void SmiOperation(Result* operand, Make sure to put a comment in the header file mentioning whether the Result* will be consumed by the operation (in this case, I think it is).
I think the code to free eax and edx has turned out very nicely. It starts at codegen-ia32.cc:5400. See what you think. We need to go from the situation where "answer" is a free register that is a copy of left to the situation where eax and edx are free registers, and eax is a copy of left. Left and right are arbitrary register results.
The register shuffling code looks good. I'm not sure if there's any good way to abstract it or if we just handle it on a case-by-case basis. We'll see as we hit more cases like this. http://codereview.chromium.org/21538/diff/2001/2003 File src/codegen-ia32.cc (right): http://codereview.chromium.org/21538/diff/2001/2003#newcode753 Line 753: Result GenerateInlineCode(Result* left, Result* right); This needs a comment both here and at the implementation telling whether the results "left" and "right" are consumed. http://codereview.chromium.org/21538/diff/2001/2003#newcode833 Line 833: if (!right_const_smi) { I'd rather not see negated conditions when there is both a then and else part. It seems more straightforward to swap them. http://codereview.chromium.org/21538/diff/2001/2003#newcode929 Line 929: if (answer_object == Heap::undefined_value_) { Use the accessor Heap::undefined_value(). http://codereview.chromium.org/21538/diff/2001/2003#newcode932 Line 932: Result answer_result(Handle<Object *>(answer_object), this); You mean Handle<Object>? No space between "Object" and "*". Don't we have a VirtualFrame::Push member function that takes a handle anyway, so there's no need to build an result. I think that explicitly constructing a Result object should be treated as a bad smell. http://codereview.chromium.org/21538/diff/2001/2003#newcode5438 Line 5438: __ mov(answer.reg(), Operand(eax)); Use the register-register encoding of mov, not the register-operand one. http://codereview.chromium.org/21538/diff/2001/2003#newcode5467 Line 5467: __ mov(answer.reg(), Operand(edx)); Ditto. http://codereview.chromium.org/21538/diff/2001/2003#newcode5477 Line 5477: __ mov(eax, Operand(left.reg())); Ditto. http://codereview.chromium.org/21538/diff/2001/2002 File src/codegen-ia32.h (right): http://codereview.chromium.org/21538/diff/2001/2002#newcode483 Line 483: void SmiOperation(Result* operand, This needs a comment in the header telling whether it consumes the result "operand" or not.
Still debugging - not ready for final review. http://codereview.chromium.org/21538/diff/2001/2003 File src/codegen-ia32.cc (right): http://codereview.chromium.org/21538/diff/2001/2003#newcode753 Line 753: Result GenerateInlineCode(Result* left, Result* right); On 2009/02/23 09:10:16, Kevin Millikin wrote: > This needs a comment both here and at the implementation telling whether the > results "left" and "right" are consumed. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode833 Line 833: if (!right_const_smi) { On 2009/02/23 09:10:16, Kevin Millikin wrote: > I'd rather not see negated conditions when there is both a then and else part. > It seems more straightforward to swap them. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode929 Line 929: if (answer_object == Heap::undefined_value_) { On 2009/02/23 09:10:16, Kevin Millikin wrote: > Use the accessor Heap::undefined_value(). Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode932 Line 932: Result answer_result(Handle<Object *>(answer_object), this); On 2009/02/23 09:10:16, Kevin Millikin wrote: > You mean Handle<Object>? No space between "Object" and "*". Don't we have a > VirtualFrame::Push member function that takes a handle anyway, so there's no > need to build an result. > > I think that explicitly constructing a Result object should be treated as a bad > smell. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode5438 Line 5438: __ mov(answer.reg(), Operand(eax)); On 2009/02/23 09:10:16, Kevin Millikin wrote: > Use the register-register encoding of mov, not the register-operand one. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode5467 Line 5467: __ mov(answer.reg(), Operand(edx)); On 2009/02/23 09:10:16, Kevin Millikin wrote: > Ditto. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode5477 Line 5477: __ mov(eax, Operand(left.reg())); On 2009/02/23 09:10:16, Kevin Millikin wrote: > Ditto. Done. http://codereview.chromium.org/21538/diff/2001/2002 File src/codegen-ia32.h (right): http://codereview.chromium.org/21538/diff/2001/2002#newcode483 Line 483: void SmiOperation(Result* operand, On 2009/02/23 09:10:16, Kevin Millikin wrote: > This needs a comment in the header telling whether it consumes the result > "operand" or not. Done.
Here is the changelist. Please review it. Thanks.
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/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.
This changelist has moved from experimental compiler branch to bleeding-edge, with a new issue number, http://codereview.chromium.org/42006. |
