Unboxed load/store indexed of Float32x4

Unboxed load/store indexed of Float32x4

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

[Cutch, 2013-04-09 10:30:51]

Adding BoxFloat32x4Instr, UnboxFloat32x4Instr, and support for inlined load and
store from typed array views.

[Cutch, 2013-04-10 07:53:27]

$ ./tools/test.py -m all -a all
Test configurations: none_vm_debug_ia32 none_vm_release_ia32 none_vm_debug_x64
none_vm_release_x64 none_vm_debug_simarm none_vm_release_simarm
none_vm_debug_simmips none_vm_release_simmips
[09:08 | 100% | +28101 | -    0]

[Cutch, 2013-04-10 11:13:31]

Patch Set 4 includes many tests which trigger deoptimizations.

[srdjan, 2013-04-10 15:33:23]

LGTM after verifying that the tests deoptimize as expected (with
--trace-deoptimization, --trace-compiler) and after Slava's LGTM

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:3743: const intptr_t token_pos_;
Why do we need token_pos_ (here and in BoxDoubleInstr).

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2741: if (EAX != locs->out().reg()) __
movl(locs->out().reg(), EAX);
Use MoveRegister(locs->out().reg(), EAX), it emits code only if registers are
different.

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:2611: if (RAX != locs->out().reg()) __
movq(locs->out().reg(), RAX);
ditto

https://codereview.chromium.org/13818006/diff/12001/tests/lib/typeddata/float...
File tests/lib/typeddata/float32x4_list_test.dart (right):

https://codereview.chromium.org/13818006/diff/12001/tests/lib/typeddata/float...
tests/lib/typeddata/float32x4_list_test.dart:29: }
s/Deop/Deopt/ below.

[Cutch, 2013-04-10 16:36:13]

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:3743: const intptr_t token_pos_;
On 2013/04/10 15:33:23, srdjan wrote:
> Why do we need token_pos_ (here and in BoxDoubleInstr).

We don't. instance_call was always NULL. I've removed it from both. They now
match BoxIntegerInstr.

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2741: if (EAX != locs->out().reg()) __
movl(locs->out().reg(), EAX);
On 2013/04/10 15:33:23, srdjan wrote:
> Use MoveRegister(locs->out().reg(), EAX), it emits code only if registers are
> different.

Done. I also made this change in the BoxDoubleSlowPath and BoxIntegerSlowPath.

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/13818006/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:2611: if (RAX != locs->out().reg()) __
movq(locs->out().reg(), RAX);
On 2013/04/10 15:33:23, srdjan wrote:
> ditto

Done.

https://codereview.chromium.org/13818006/diff/12001/tests/lib/typeddata/float...
File tests/lib/typeddata/float32x4_list_test.dart (right):

https://codereview.chromium.org/13818006/diff/12001/tests/lib/typeddata/float...
tests/lib/typeddata/float32x4_list_test.dart:29: }
On 2013/04/10 15:33:23, srdjan wrote:
> s/Deop/Deopt/ below.

Done.

[Cutch, 2013-04-10 16:44:49]

Codereview doesn't allow for pasting the amount of text generated with
--trace_compiler and --trace_deoptimization but I've emailed both you and Slava.

[Cutch, 2013-04-10 16:51:08]

Against patch set 5:

$ ./tools/test.py -m all -a all --checked
Test configurations: none_vm_debug_ia32_checked none_vm_release_ia32_checked
none_vm_debug_x64_checked none_vm_release_x64_checked
none_vm_debug_simarm_checked none_vm_release_simarm_checked
none_vm_debug_simmips_checked none_vm_release_simmips_checked
[09:52 | 100% | +28092 | -    0]

[Vyacheslav Egorov (Google), 2013-04-10 20:00:48]

LGTM with comments addressed.

There is a bug that can lead to a crash.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2630: compiler->GenerateCall(0,  // No
token position.
Scanner::kDummyTokenIndex

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2737: compiler->GenerateCall(0,  // No
token position.
Scanner::kDummyTokenIndex

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2785: __ movups(result,
FieldAddress(value, Float32x4::value_offset()));
move movups down and make if (value_cid != kFloat32x4Cid) ...

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2789: __ CompareClassId(value,
kFloat32x4Cid, temp);
This will crash if value is a Smi.

This is also just a normal CheckClass. 

Is there any reason why it is not split out into CheckClass (and CheckNonSmi)
during IR construction? I would suggest to generate

CheckNonSmi v1
CheckClass v1, kFloat32x4Cid
v2 = UnboxFloat32x4 v1

(in UnboxDouble handles Smis & Doubles that is why it does not split checks
out).

Given this bug consider writing a regression test that passes smi to a place
that tries to unbox float32x4 to see that it'll crash.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:3292: compiler->GenerateCall(0,  // No
token position.
Scanner::kDummyTokenIndex

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:2505: compiler->GenerateCall(0,  // No
token position.
Scanner::kDummyTokenIndex

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:2650: const intptr_t value_cid =
value()->Type()->ToCid();
Same comments from ia32 apply here.

https://codereview.chromium.org/13818006/diff/22001/tests/lib/typeddata/float...
File tests/lib/typeddata/float32x4_list_test.dart (right):

https://codereview.chromium.org/13818006/diff/22001/tests/lib/typeddata/float...
tests/lib/typeddata/float32x4_list_test.dart:87: }
As noted in the other comments add Smi value case.

Also take care not too exceed a deoptimization limit so that we stop optimizing
the function. 

Maybe bump it up to big number via
VMFlags:--deoptimization_counter_threshold=1000

[Cutch, 2013-04-11 09:39:06]

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2630: compiler->GenerateCall(0,  // No
token position.
On 2013/04/10 20:00:48, Vyacheslav Egorov (Google) wrote:
> Scanner::kDummyTokenIndex

Done.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2737: compiler->GenerateCall(0,  // No
token position.
On 2013/04/10 20:00:48, Vyacheslav Egorov (Google) wrote:
> Scanner::kDummyTokenIndex

Done.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2785: __ movups(result,
FieldAddress(value, Float32x4::value_offset()));
On 2013/04/10 20:00:48, Vyacheslav Egorov (Google) wrote:
> move movups down and make if (value_cid != kFloat32x4Cid) ...

Done.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2789: __ CompareClassId(value,
kFloat32x4Cid, temp);
I've added a CheckNonSmi instruction and insert it into the IR in
InsertConversion. 

I'm not sure how to insert a class check in InsertConversion because I don't
have access to the ICData. So, I've kept the type check in UnboxFloat32x4Instr.

Currently there will be no unboxing done on Float32x4 values except in a store
indexed, which already does a ClassCheck for the value.

I have included a regression test though and proper type checking support for
--checked mode in Float32x4List store indexes.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:3292: compiler->GenerateCall(0,  // No
token position.
On 2013/04/10 20:00:48, Vyacheslav Egorov (Google) wrote:
> Scanner::kDummyTokenIndex

Done.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:2505: compiler->GenerateCall(0,  // No
token position.
On 2013/04/10 20:00:48, Vyacheslav Egorov (Google) wrote:
> Scanner::kDummyTokenIndex

Done.

https://codereview.chromium.org/13818006/diff/22001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:2650: const intptr_t value_cid =
value()->Type()->ToCid();
On 2013/04/10 20:00:48, Vyacheslav Egorov (Google) wrote:
> Same comments from ia32 apply here.

See my comments in ia32.

https://codereview.chromium.org/13818006/diff/22001/tests/lib/typeddata/float...
File tests/lib/typeddata/float32x4_list_test.dart (right):

https://codereview.chromium.org/13818006/diff/22001/tests/lib/typeddata/float...
tests/lib/typeddata/float32x4_list_test.dart:87: }
Added a separate test which attempts to store a smi value. Note that store
indexed already inserts a class check, which stops it from crashing. 

I've added the VMOptions= setting.

[Vyacheslav Egorov (Google), 2013-04-11 11:29:35]

CheckSmi is removed because CheckClass actually guarantees that value is not
smi. 

I am sorry for confusing you.

LGTM with comments addressed.

https://codereview.chromium.org/13818006/diff/28001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13818006/diff/28001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2784: if (value_cid != kFloat32x4Cid) {
Indeed right now we build IR in a way that guarantees that value's class id is
checked already. 

In this case I suggest you assert here: that value_cid == kFloat32x4Cid and
remove this code.

https://codereview.chromium.org/13818006/diff/28001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:3164: LocationSummary*
CheckNonSmiInstr::MakeLocationSummary() const {
Oh, sorry. My brain confused between V8's HIR and Dart IR. In Dart IR we have
CheckNonSmi folded into CheckClass so you don't need it. 

You can remove this instruction.

[Cutch, 2013-04-11 12:00:59]

https://codereview.chromium.org/13818006/diff/28001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13818006/diff/28001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2784: if (value_cid != kFloat32x4Cid) {
On 2013/04/11 11:29:35, Vyacheslav Egorov (Google) wrote:
> Indeed right now we build IR in a way that guarantees that value's class id is
> checked already. 
> 
> In this case I suggest you assert here: that value_cid == kFloat32x4Cid and
> remove this code.

Done.

https://codereview.chromium.org/13818006/diff/28001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:3164: LocationSummary*
CheckNonSmiInstr::MakeLocationSummary() const {
On 2013/04/11 11:29:35, Vyacheslav Egorov (Google) wrote:
> Oh, sorry. My brain confused between V8's HIR and Dart IR. In Dart IR we have
> CheckNonSmi folded into CheckClass so you don't need it. 
> 
> You can remove this instruction. 

Done.

[Cutch, 2013-04-11 12:22:47]

On Patch Set 8:

$ ./tools/test.py -m all -a all --checked
Test configurations: none_vm_debug_ia32_checked none_vm_release_ia32_checked
none_vm_debug_x64_checked none_vm_release_x64_checked
none_vm_debug_simarm_checked none_vm_release_simarm_checked
none_vm_debug_simmips_checked none_vm_release_simmips_checked
[09:25 | 100% | +28130 | -    0]

[Cutch, 2013-04-11 12:51:00]

Committed patchset #8 manually as r21274 (presubmit successful).