Added fast case for shift operations when the left parameter is a double and ...

Added fast case for shift operations when the left parameter is a double and we know that the right parameter is a smi.

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

[Lasse Reichstein, 2010-03-15 08:45:58]

LGTM

http://codereview.chromium.org/955001/diff/3001/4001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/955001/diff/3001/4001#newcode1445
src/ia32/codegen-ia32.cc:1445: right->number_info().IsSmi()) {
What about the case where left->number_info().IsSmi()? Could you skip some of
the code in that case?

http://codereview.chromium.org/955001/diff/3001/4001#newcode1474
src/ia32/codegen-ia32.cc:1474: __ SmiUntag(answer.reg());
Would it be worth not untagging if doing right shifts, and just increase the
shift amount?
Probably not (you would have to test the increment of ecx for overflowing).

http://codereview.chromium.org/955001/diff/3001/4001#newcode1514
src/ia32/codegen-ia32.cc:1514: __ test(answer.reg(), Immediate(0xc0000000));
This test is correct for SHR, but overly restrictive for the other shifts (those
with signed 32-bit results).
For those do:
 __ cmp(answer.reg(), Immediate(0xc0000000));
 __ deferred->Branch(negative);
Using test instead would miss the negative smi results.

Also, please do a STATIC_ASSERT(kSmiValueSize == 31) to document what you are
testing for (which are pretty likely for both ASR and SHL).

http://codereview.chromium.org/955001/diff/3001/4001#newcode1533
src/ia32/codegen-ia32.cc:1533: }
You are duplicating code from this branch.
Maybe move your code inside this branch with an inner 
 if (has SSS2 and right is smi) 

There are a lot of different cases that boil down to pretty much the same code:
Either number can be a smi (known or not), heap number or non-number. You can
convert smis and heap-numbers to int32, and the code from there is pretty much
identical (except how you restore the numbers if smi-ops fail).

[Rico, 2010-03-15 12:57:00]

As suggested I changed this to be a special case in the existing shift logic
(the special case where right is known to be a smi and we have SSE2 support).

http://codereview.chromium.org/955001/diff/3001/4001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/955001/diff/3001/4001#newcode1445
src/ia32/codegen-ia32.cc:1445: right->number_info().IsSmi()) {
On 2010/03/15 08:45:58, Lasse Reichstein wrote:
> What about the case where left->number_info().IsSmi()? Could you skip some of
> the code in that case?

Done.

http://codereview.chromium.org/955001/diff/3001/4001#newcode1474
src/ia32/codegen-ia32.cc:1474: __ SmiUntag(answer.reg());
On 2010/03/15 08:45:58, Lasse Reichstein wrote:
> Would it be worth not untagging if doing right shifts, and just increase the
> shift amount?
> Probably not (you would have to test the increment of ecx for overflowing).

Not relevant in new version.

http://codereview.chromium.org/955001/diff/3001/4001#newcode1514
src/ia32/codegen-ia32.cc:1514: __ test(answer.reg(), Immediate(0xc0000000));
On 2010/03/15 08:45:58, Lasse Reichstein wrote:
> This test is correct for SHR, but overly restrictive for the other shifts
(those
> with signed 32-bit results).
> For those do:
>  __ cmp(answer.reg(), Immediate(0xc0000000));
>  __ deferred->Branch(negative);
> Using test instead would miss the negative smi results.
> 
> Also, please do a STATIC_ASSERT(kSmiValueSize == 31) to document what you are
> testing for (which are pretty likely for both ASR and SHL).

Not in new version

[Lasse Reichstein, 2010-03-15 14:13:25]

LGTM

http://codereview.chromium.org/955001/diff/4002/11001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/955001/diff/4002/11001#newcode1502
src/ia32/codegen-ia32.cc:1502: 
Isn't this dead code if the above if is true?
I.e., put it in an else branch.
That would eliminate the last jump as well.

[Rico, 2010-03-15 14:18:29]

http://codereview.chromium.org/955001/diff/4002/11001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/955001/diff/4002/11001#newcode1502
src/ia32/codegen-ia32.cc:1502: 
On 2010/03/15 14:13:25, Lasse Reichstein wrote:
> Isn't this dead code if the above if is true?
> I.e., put it in an else branch.
> That would eliminate the last jump as well.

Done.