Inline binary Float32x4 ops.

Binary Float32x4 ops.

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

[Cutch, 2013-04-14 15:22:45]

Slava,

I'm wondering why I had to adjust my Unbox instructions to be different than you
suggested here: https://codereview.chromium.org/13818006/

Thanks,
John

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

LGTM with comments addressed.

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

https://codereview.chromium.org/13867006/diff/1/runtime/vm/deopt_instructions...
runtime/vm/deopt_instructions.cc:825: intptr_t from_index =
(from_loc.stack_index() < 0) ?
This code is duplicated three times now. Consider making a helper method.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1089: ReplaceCall(call, float32x4_bin_op);
You also need to emit CheckClass here just like you do for the store into typed
data.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_type_pr...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:968: 
add an empty line

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:1090: Definition*
BinaryFloat32x4OpInstr::Canonicalize(
This matches default behavior. No need ot override

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:2790: if (value_cid != kFloat32x4Cid) {
See the comment in the optimizer. Emit CheckClass for operands of the binary
operation and restore assert here. 

It should never trigger.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:2801: if (value_cid != kFloat32x4Cid) {
Ditto

[Cutch, 2013-04-15 13:34:56]

$ ./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:36 | 100% | +28210 | -    0]
=== Failure summary:

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

https://codereview.chromium.org/13867006/diff/1/runtime/vm/deopt_instructions...
runtime/vm/deopt_instructions.cc:825: intptr_t from_index =
(from_loc.stack_index() < 0) ?
On 2013/04/15 11:09:35, Vyacheslav Egorov (Google) wrote:
> This code is duplicated three times now. Consider making a helper method.

Done.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1089: ReplaceCall(call, float32x4_bin_op);
On 2013/04/15 11:09:35, Vyacheslav Egorov (Google) wrote:
> You also need to emit CheckClass here just like you do for the store into
typed
> data.

Done.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_type_pr...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:968: 
On 2013/04/15 11:09:35, Vyacheslav Egorov (Google) wrote:
> add an empty line

Done.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:1090: Definition*
BinaryFloat32x4OpInstr::Canonicalize(
On 2013/04/15 11:09:35, Vyacheslav Egorov (Google) wrote:
> This matches default behavior. No need ot override 

Done.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:2790: if (value_cid != kFloat32x4Cid) {
On 2013/04/15 11:09:35, Vyacheslav Egorov (Google) wrote:
> See the comment in the optimizer. Emit CheckClass for operands of the binary
> operation and restore assert here. 
> 
> It should never trigger.

Done.

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/13867006/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:2801: if (value_cid != kFloat32x4Cid) {
On 2013/04/15 11:09:35, Vyacheslav Egorov (Google) wrote:
> Ditto

Done.

[Vyacheslav Egorov (Google), 2013-04-15 13:49:21]

lgtm

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/deopt_instructi...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/deopt_instructi...
runtime/vm/deopt_instructions.cc:767: 
add empty line

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1092: InsertBefore(call, check_class,
call->env(), Definition::kEffect);
AddCheckClass(left->definition(),
              call->ic_data()->AsUnaryClassChecksForArgNr(0),
              call->deopt_id(),
              call->env(),
              call);

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1098: InsertBefore(call, check_class,
call->env(), Definition::kEffect);
AddCheckClass(right->definition(),
              call->ic_data()->AsUnaryClassChecksForArgNr(1),
              call->deopt_id(),
              call->env(),
              call);

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:2779: }
I think this temp should go away now.

[Cutch, 2013-04-15 14:07:06]

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/deopt_instructi...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/deopt_instructi...
runtime/vm/deopt_instructions.cc:767: 
On 2013/04/15 13:49:21, Vyacheslav Egorov (Google) wrote:
> add empty line

Done.

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1092: InsertBefore(call, check_class,
call->env(), Definition::kEffect);
On 2013/04/15 13:49:21, Vyacheslav Egorov (Google) wrote:
> AddCheckClass(left->definition(),
>               call->ic_data()->AsUnaryClassChecksForArgNr(0),
>               call->deopt_id(),
>               call->env(),
>               call);

Done.

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1098: InsertBefore(call, check_class,
call->env(), Definition::kEffect);
On 2013/04/15 13:49:21, Vyacheslav Egorov (Google) wrote:
> AddCheckClass(right->definition(),
>               call->ic_data()->AsUnaryClassChecksForArgNr(1),
>               call->deopt_id(),
>               call->env(),
>               call);

Done.

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/13867006/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:2779: }
On 2013/04/15 13:49:21, Vyacheslav Egorov (Google) wrote:
> I think this temp should go away now.

Done.

[Cutch, 2013-04-16 10:04:42]

Committed patchset #4 manually as r21541 (presubmit successful).