Remove spills from SMI immediate bitwise and shift operations.

Remove spills from SMI immediate bitwise and shift operations.

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

[William Hesse, 2009-01-22 18:45:48]

This change currently incorporates the earlier changlist 18495.  Once that
change is committed, it will be updated to no longer include it.

[William Hesse, 2009-01-23 16:22:51]

This changelist is now updated, and should be reviewed.  It no longer includes
changes from changelist 18495 as part of its delta.

On 2009/01/22 18:45:48, William Hesse wrote:
> This change currently incorporates the earlier changlist 18495.  Once that
> change is committed, it will be updated to no longer include it.

[Kevin Millikin (Chromium), 2009-01-24 08:00:12]

Initial round of comments.  I have not looked closely at the whole thing yet so
there will be more.

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

http://codereview.chromium.org/18669/diff/203/3#newcode806
Line 806: DeferredCode(generator), op_(op), value_(value),
Since you're cleaning up this code, can you move the initializers to one per
line?

http://codereview.chromium.org/18669/diff/203/3#newcode1048
Line 1048: VirtualFrame::SpilledScope spilled_scope(this);
Remove this spilled scope.

http://codereview.chromium.org/18669/diff/203/3#newcode1051
Line 1051: new DeferredInlinedSmiOperation(this, Token::SAR, smi_value,
smi_value is not the Smi tagged version of shift_value.  It seems like you have
to pass Smi::FromInt(shift_value) to match the previous code.

http://codereview.chromium.org/18669/diff/203/3#newcode1073
Line 1073: VirtualFrame::SpilledScope spilled_scope(this);
Remove this spilled scope.

http://codereview.chromium.org/18669/diff/203/3#newcode1076
Line 1076: new DeferredInlinedSmiOperation(this, Token::SHR, smi_value,
smi_value ==> Smi::FromInt(shift_value)

[William Hesse, 2009-01-26 09:52:10]

Updated file uploaded.  Comments addressed.

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

http://codereview.chromium.org/18669/diff/203/3#newcode806
Line 806: DeferredCode(generator), op_(op), value_(value),
On 2009/01/24 08:00:13, Kevin Millikin wrote:
> Since you're cleaning up this code, can you move the initializers to one per
> line?

Done.

http://codereview.chromium.org/18669/diff/203/3#newcode1051
Line 1051: new DeferredInlinedSmiOperation(this, Token::SAR, smi_value,
On 2009/01/24 08:00:13, Kevin Millikin wrote:
> smi_value is not the Smi tagged version of shift_value.  It seems like you
have
> to pass Smi::FromInt(shift_value) to match the previous code.

The masking with 0x1f is performed within the slow case, in runtime.cc, matching
what is done in the fast case here.  Comment added.

http://codereview.chromium.org/18669/diff/203/3#newcode1073
Line 1073: VirtualFrame::SpilledScope spilled_scope(this);
On 2009/01/24 08:00:13, Kevin Millikin wrote:
> Remove this spilled scope.

Done.

http://codereview.chromium.org/18669/diff/203/3#newcode1076
Line 1076: new DeferredInlinedSmiOperation(this, Token::SHR, smi_value,
On 2009/01/24 08:00:13, Kevin Millikin wrote:
> smi_value ==> Smi::FromInt(shift_value)

Not needed.  Commented.

[Kevin Millikin (Chromium), 2009-01-26 09:55:51]

OK, I've been through it.  Just a few comments or questions.

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

http://codereview.chromium.org/18669/diff/203/3#newcode1087
Line 1087: if (shift_value < 2) {
This needs a comment so that 2 doesn't seem so magic.

http://codereview.chromium.org/18669/diff/203/3#newcode1092
Line 1092: // tag result and store it in TOS (eax)
Remove this comment.

http://codereview.chromium.org/18669/diff/203/3#newcode1112
Line 1112: new DeferredInlinedSmiOperation(this, Token::SHL, smi_value,
Smi::FromInt(shift_value)?

http://codereview.chromium.org/18669/diff/203/3#newcode1124
Line 1124: } else if (shift_value > 1) {
Is this an "else if" or a simple else (possibly with an assert that shift_value
> 1)?

http://codereview.chromium.org/18669/diff/203/3#newcode1127
Line 1127: // Convert int result to Smi, checking that it is in int range.
Is it possible to just shift the smi-tagged value, rather than untagging,
shifting, and retagging?

[William Hesse, 2009-01-26 11:49:32]

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

http://codereview.chromium.org/18669/diff/203/3#newcode1087
Line 1087: if (shift_value < 2) {
On 2009/01/26 09:55:51, Kevin Millikin wrote:
> This needs a comment so that 2 doesn't seem so magic.

Done.

http://codereview.chromium.org/18669/diff/203/3#newcode1092
Line 1092: // tag result and store it in TOS (eax)
On 2009/01/26 09:55:51, Kevin Millikin wrote:
> Remove this comment.

Done.

http://codereview.chromium.org/18669/diff/203/3#newcode1112
Line 1112: new DeferredInlinedSmiOperation(this, Token::SHL, smi_value,
On 2009/01/26 09:55:51, Kevin Millikin wrote:
> Smi::FromInt(shift_value)?

Not needed.

http://codereview.chromium.org/18669/diff/203/3#newcode1124
Line 1124: } else if (shift_value > 1) {
On 2009/01/26 09:55:51, Kevin Millikin wrote:
> Is this an "else if" or a simple else (possibly with an assert that
shift_value
> > 1)?

It is really an else if, and the shift_value == 1 case is now commented.

http://codereview.chromium.org/18669/diff/203/3#newcode1127
Line 1127: // Convert int result to Smi, checking that it is in int range.
On 2009/01/26 09:55:51, Kevin Millikin wrote:
> Is it possible to just shift the smi-tagged value, rather than untagging,
> shifting, and retagging?

We have dropped untagging except in the shift 0 case, but shifting and retagging
need to be separate, for the Smi test to be combined with retagging.

[Kevin Millikin (Chromium), 2009-01-26 16:00:55]

LGTM (remember to check the buildbot after submitting!).