Intrinsify natural word size typed array getters on IA32 and X64

Intrinsify natural world typed array getters

Committed: https://code.google.com/p/dart/source/detail?r=13696

[cshapiro, 2012-10-12 22:33:52]

looks good to me, worth getting srdjan's assessment as well

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32.cc
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
runtime/vm/intrinsifier_ia32.cc:596: // Verify that the unsigned value in EAX
can be stored in a Smi.
Indentation...

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
runtime/vm/intrinsifier_ia32.cc:597: __ shrl(EBX, Immediate(30));
Please use the named constant kSmiBits instead of the number 30.

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_x64.cc
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_x64.c...
runtime/vm/intrinsifier_x64.cc:567: __ shrq(R12, Immediate(62));
Please use the named constant kSmiBits instead of the number 62.

[srdjan, 2012-10-12 22:52:48]

DBC

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32.cc
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
runtime/vm/intrinsifier_ia32.cc:576: // Verify that the signed value in EBX can
be stored in a Smi.
Quicker way is to test with:

        __ testl(EAX, Immediate(0xc0000000));
        __ j(negative, &fall_through);

See DoubleToInt.

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
runtime/vm/intrinsifier_ia32.cc:598: __ j(NOT_ZERO, &fall_through,
Assembler::kNearJump);  // Won't fit Smi.
I believe quicker test for unsigned is:

__ testl(EAX, Immediate(0xc0000000));

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
runtime/vm/intrinsifier_ia32.cc:606: bool
Intrinsifier::Int64Array_getIndexed(Assembler* assembler) {
You could add here code that checks if the result fits into Smi and return Smi
if so.

[Cutch, 2012-10-13 18:46:31]

On 2012/10/12 22:52:48, srdjan wrote:
> DBC
> 
>
https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32.cc
> File runtime/vm/intrinsifier_ia32.cc (right):
> 
>
https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
> runtime/vm/intrinsifier_ia32.cc:576: // Verify that the signed value in EBX
can
> be stored in a Smi.
> Quicker way is to test with:
> 
>         __ testl(EAX, Immediate(0xc0000000));
>         __ j(negative, &fall_through);
> 
> See DoubleToInt.
> 

Carl and I were optimizing for code size and wanted to avoid emitting the large
immediate constant in the instruction stream. My original patch had the code
you've recommended. What implementation should we use?

>
https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
> runtime/vm/intrinsifier_ia32.cc:598: __ j(NOT_ZERO, &fall_through,
> Assembler::kNearJump);  // Won't fit Smi.
> I believe quicker test for unsigned is:
> 
> __ testl(EAX, Immediate(0xc0000000));

Again, my original patch included the constant and worked like this. This is
starting to go in circles- can we pick one method and go with it?

> 
>
https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
> runtime/vm/intrinsifier_ia32.cc:606: bool
> Intrinsifier::Int64Array_getIndexed(Assembler* assembler) {
> You could add here code that checks if the result fits into Smi and return Smi
> if so.


Agreed. Is this the preferred method?

Load into an XMM register
Determine if it can fit inside a Smi
If so, return a Smi
Else, fallback

Thanks,
John

[Florian Schneider, 2012-10-15 14:29:16]

DBC

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32.cc
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/11139004/diff/1/runtime/vm/intrinsifier_ia32....
runtime/vm/intrinsifier_ia32.cc:576: // Verify that the signed value in EBX can
be stored in a Smi.
On 2012/10/12 22:52:48, srdjan wrote:
> Quicker way is to test with:
> 
>         __ testl(EAX, Immediate(0xc0000000));
>         __ j(negative, &fall_through);
> 
> See DoubleToInt.

For signed ints it should be 

__ cmpl(EAX, Immediate(0xc0000000));

I'm fine with either version (shl or cmpl) here, since we can afford to use a
temp register.

[Cutch, 2012-10-15 18:26:41]

All comments should be addressed. On IA32 the immediate constants are used while
on X64 shifts are used. Loading 64-bit values on IA32 will wait for the next CL.

[cshapiro, 2012-10-15 21:41:58]

lgtm, anyone else?

[Cutch, 2012-10-16 18:28:06]

Ran test suites:

dhcp-172-19-9-227:runtime johnmccutchan$ ../tools/test.py 
Test configuration: none_vm_debug_ia32
[06:24 | 100% | + 5853 | -    0]

dhcp-172-19-9-227:runtime johnmccutchan$ ../tools/test.py -a x64
Test configuration: none_vm_debug_x64
[05:58 | 100% | + 5822 | -    0]

Uploading.