Optimize binary operations in which one or both operands is a constant smi.

Optimize binary operations in which one or both operands is a constant smi.

Committed: http://code.google.com/p/v8/source/detail?r=1621

[William Hesse, 2009-03-10 09:32:26]

This changelist is a continuation of changelist
http://codereview.chromium.org/21538,
which is moved from the experimental compiler branch
to bleeding-edge to make this new issue.

This change has been reviewed extensively under the old number, and passes all
tests.

[Kevin Millikin (Chromium), 2009-03-10 10:14:47]

Looks good.  A few comments.

http://codereview.chromium.org/42006/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/42006/diff/1/3#newcode686
Line 686: // should be inlined, placed in the stub, or omitted entirely.
To parallel the order of the enum, "should be placed in the stub, inlined, or
omitted entirely".

http://codereview.chromium.org/42006/diff/1/3#newcode722
Line 722: class OpBits: public BitField<Token::Value, 2, 12> {};
You might consider asserting in the constructor that NUM_TOKENS fits in 12 bits.

http://codereview.chromium.org/42006/diff/1/3#newcode834
Line 834: // Compute the result, and return that as a constant on the frame.
Maybe should avoid words like "return" because they can be ambiguous (return
from this C++ function?  return at runtime?).  "Compute the result and leave it
on the frame."?

http://codereview.chromium.org/42006/diff/1/3#newcode845
Line 845: false, overwrite_mode);
Wrong indentation?

http://codereview.chromium.org/42006/diff/1/3#newcode848
Line 848: ConstantSmiBinaryOperation(op, &right, left.handle(), type,
Wrong indentation?

http://codereview.chromium.org/42006/diff/1/3#newcode860
Line 860: // The same stub is used for NO_SMI_CODE and SMI_CODE_INLINED.
It's probably better to make the stub handle all three values of the enum rather
than just two.  It's less confusing here and will be probably be easier to
change later by just changing the stub.

http://codereview.chromium.org/42006/diff/1/3#newcode956
Line 956: // Create a new deferred code object that calls GenericBinaryOpStub
It calls a (specialized) instance of a GenericBinaryOpStub, not the
GenericBinaryOpStub.  (But I don't think you even need to mention that here,
because this non-local comment becomes out of date if the deferred code
changes).

http://codereview.chromium.org/42006/diff/1/3#newcode1177
Line 1177: // Consumes the argument "operand".
You might consider asserting valid entry registers here, because this function
assumes that register allocation will not fail.

http://codereview.chromium.org/42006/diff/1/3#newcode1220
Line 1220: Result answer(this);  // Only allocated a new register if reversed.
"allocated" -> "allocate"

http://codereview.chromium.org/42006/diff/1/3#newcode1267
Line 1267: if (shift_value > 0) {
In general it might be a good idea to try to make sure we have tests that hit
these special code paths.

http://codereview.chromium.org/42006/diff/1/3#newcode5608
Line 5608: // registers).
You might consider asserting valid entry registers here.

http://codereview.chromium.org/42006/diff/1/3#newcode5620
Line 5620: __ or_(answer.reg(), Operand(right->reg()));
(As a future change, we could simplify the smi check code if left and right are
the same registers, eg, "x + x" or "y = x; x + y".)

http://codereview.chromium.org/42006/diff/1/2
File src/codegen-ia32.h (right):

http://codereview.chromium.org/42006/diff/1/2#newcode486
Line 486: // Emit code to perform a binary operation on
Blank line between functions in the header file, please.

http://codereview.chromium.org/42006/diff/1/2#newcode496
Line 496: // two likely smis.  The code to handle smi arguments is produced
inline.
Weird word wrapping (very short line followed by very long line, makes me wonder
why).  For consistency, put variable names in double quotes always or never (I
don't think they need it).

[iposva, 2009-03-10 14:27:06]

LGTM when Kevin's comments are addressed plus some more comments.

Also what is the impact on compilation time and benchmark scores on top of the
code bloat question in the comments?

Thanks,
-Ivan

http://codereview.chromium.org/42006/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/42006/diff/1/3#newcode729
Line 729: ModeBits::encode(mode_) |
I know this is not yours, but this looks like broken indentation. Thanks!

http://codereview.chromium.org/42006/diff/1/3#newcode960
Line 960: SMI_CODE_INLINED);
Since this code is now generated even if not in a loop in certain situations
(e.g. reversed constant smi bitops), do you have measured the impact on code
bloat?

[William Hesse, 2009-03-26 12:55:46]

The size of compiled code does not seem to change much on the V8 benchmarks, and
compilation time does not seem to change on Parsy.

http://codereview.chromium.org/42006/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/42006/diff/1/3#newcode686
Line 686: // should be inlined, placed in the stub, or omitted entirely.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> To parallel the order of the enum, "should be placed in the stub, inlined, or
> omitted entirely".

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode722
Line 722: class OpBits: public BitField<Token::Value, 2, 12> {};
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> You might consider asserting in the constructor that NUM_TOKENS fits in 12
bits.

This assertion is done in BitField::encode(), which is called by MinorKey(),
which I think is sufficient.

http://codereview.chromium.org/42006/diff/1/3#newcode729
Line 729: ModeBits::encode(mode_) |
On 2009/03/10 14:27:06, iposva wrote:
> I know this is not yours, but this looks like broken indentation. Thanks!

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode834
Line 834: // Compute the result, and return that as a constant on the frame.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> Maybe should avoid words like "return" because they can be ambiguous (return
> from this C++ function?  return at runtime?).  "Compute the result and leave
it
> on the frame."?

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode845
Line 845: false, overwrite_mode);
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> Wrong indentation?

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode848
Line 848: ConstantSmiBinaryOperation(op, &right, left.handle(), type,
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> Wrong indentation?

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode860
Line 860: // The same stub is used for NO_SMI_CODE and SMI_CODE_INLINED.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> It's probably better to make the stub handle all three values of the enum
rather
> than just two.  It's less confusing here and will be probably be easier to
> change later by just changing the stub.

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode956
Line 956: // Create a new deferred code object that calls GenericBinaryOpStub
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> It calls a (specialized) instance of a GenericBinaryOpStub, not the
> GenericBinaryOpStub.  (But I don't think you even need to mention that here,
> because this non-local comment becomes out of date if the deferred code
> changes).

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode1177
Line 1177: // Consumes the argument "operand".
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> You might consider asserting valid entry registers here, because this function
> assumes that register allocation will not fail.

We can't do that because we have a register used by the
Result operand.

http://codereview.chromium.org/42006/diff/1/3#newcode1220
Line 1220: Result answer(this);  // Only allocated a new register if reversed.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> "allocated" -> "allocate"

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode5608
Line 5608: // registers).
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> You might consider asserting valid entry registers here.

Left and right might be in registers, so we may have registers held by results
here.

http://codereview.chromium.org/42006/diff/1/3#newcode5620
Line 5620: __ or_(answer.reg(), Operand(right->reg()));
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> (As a future change, we could simplify the smi check code if left and right
are
> the same registers, eg, "x + x" or "y = x; x + y".)

Yes, we could.

http://codereview.chromium.org/42006/diff/1/2
File src/codegen-ia32.h (right):

http://codereview.chromium.org/42006/diff/1/2#newcode486
Line 486: // Emit code to perform a binary operation on
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> Blank line between functions in the header file, please.

Done.

http://codereview.chromium.org/42006/diff/1/2#newcode496
Line 496: // two likely smis.  The code to handle smi arguments is produced
inline.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> Weird word wrapping (very short line followed by very long line, makes me
wonder
> why).  For consistency, put variable names in double quotes always or never (I
> don't think they need it).

Done.

[William Hesse, 2009-03-27 11:55:28]

These changes finally made, and submitted in changelist 51007.
http://codereview.chromium.org/51007/show

http://codereview.chromium.org/42006/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/42006/diff/1/3#newcode686
Line 686: // should be inlined, placed in the stub, or omitted entirely.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> To parallel the order of the enum, "should be placed in the stub, inlined, or
> omitted entirely".

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode722
Line 722: class OpBits: public BitField<Token::Value, 2, 12> {};
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> You might consider asserting in the constructor that NUM_TOKENS fits in 12
bits.

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode834
Line 834: // Compute the result, and return that as a constant on the frame.
On 2009/03/26 12:55:46, William Hesse wrote:
> On 2009/03/10 10:14:47, Kevin Millikin wrote:
> > Maybe should avoid words like "return" because they can be ambiguous (return
> > from this C++ function?  return at runtime?).  "Compute the result and leave
> it
> > on the frame."?
> 
> Done.

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode860
Line 860: // The same stub is used for NO_SMI_CODE and SMI_CODE_INLINED.
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> It's probably better to make the stub handle all three values of the enum
rather
> than just two.  It's less confusing here and will be probably be easier to
> change later by just changing the stub.

Changed back to two values of the flags, and a separate boolean flag.

http://codereview.chromium.org/42006/diff/1/3#newcode956
Line 956: // Create a new deferred code object that calls GenericBinaryOpStub
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> It calls a (specialized) instance of a GenericBinaryOpStub, not the
> GenericBinaryOpStub.  (But I don't think you even need to mention that here,
> because this non-local comment becomes out of date if the deferred code
> changes).

Done.

http://codereview.chromium.org/42006/diff/1/3#newcode5620
Line 5620: __ or_(answer.reg(), Operand(right->reg()));
On 2009/03/10 10:14:47, Kevin Millikin wrote:
> (As a future change, we could simplify the smi check code if left and right
are
> the same registers, eg, "x + x" or "y = x; x + y".)

Done.