Inline fsin/fcos. Huge speedup on Box2D.

Inline fsin/fcos. Huge speedup on Box2D.

R=iposva@google.com

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

[Ivan Posva, 2013-08-05 23:32:16]

LGTM with mostly stylistic comments.

-Ivan

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_arm.cc:46: bool
FlowGraphCompiler::SupportsInlinedTrigonometrics() {
Please add a TODO to avoid boxing/unboxing for these kinds of functions even if
there are no direct instructions for these functions.

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_mips.cc:46: bool
FlowGraphCompiler::SupportsInlinedTrigonometrics() {
ditto

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_arm.cc:3637: ASSERT(kind() ==
MethodRecognizer::kMathSqrt);
How about:
if (kind() == MethodRecognizer::kMathSqrt) {
 ...
} else {
  UNIMPLEMENTED();
}

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:3701: }
How about

if (kind() == MethodRecognizer::kMathSqrt) {
  ...
} else if (kind() == ...) {
  ...
} else {
  UNREACHABLE();
}

to avoid returns from the middle?

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_mips.cc:3089: ASSERT(kind() ==
MethodRecognizer::kMathSqrt);
ditto

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_x64.cc:3739: if (kind() ==
MethodRecognizer::kMathSqrt) {
ditto

[srdjan, 2013-08-06 15:43:05]

Thanks!

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_arm.cc:46: bool
FlowGraphCompiler::SupportsInlinedTrigonometrics() {
On 2013/08/05 23:32:16, Ivan Posva wrote:
> Please add a TODO to avoid boxing/unboxing for these kinds of functions even
if
> there are no direct instructions for these functions.

Done.

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_mips.cc:46: bool
FlowGraphCompiler::SupportsInlinedTrigonometrics() {
On 2013/08/05 23:32:16, Ivan Posva wrote:
> ditto

Done.

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_arm.cc:3637: ASSERT(kind() ==
MethodRecognizer::kMathSqrt);
On 2013/08/05 23:32:16, Ivan Posva wrote:
> How about:
> if (kind() == MethodRecognizer::kMathSqrt) {
>  ...
> } else {
>   UNIMPLEMENTED();
> }

Done.

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:3701: }
On 2013/08/05 23:32:16, Ivan Posva wrote:
> How about
> 
> if (kind() == MethodRecognizer::kMathSqrt) {
>   ...
> } else if (kind() == ...) {
>   ...
> } else {
>   UNREACHABLE();
> }
> 
> to avoid returns from the middle?

Done.

(In many cases I prefer return from the middle if there is no code to be
executed after the if else. Otherwise, I need to read the whole method to
understand what a certain case is doing.)

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_mips.cc:3089: ASSERT(kind() ==
MethodRecognizer::kMathSqrt);
On 2013/08/05 23:32:16, Ivan Posva wrote:
> ditto

Done.

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

https://codereview.chromium.org/22266005/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_x64.cc:3739: if (kind() ==
MethodRecognizer::kMathSqrt) {
On 2013/08/05 23:32:16, Ivan Posva wrote:
> ditto

Done.

[srdjan, 2013-08-06 15:46:36]

Committed patchset #3 manually as r25816 (presubmit successful).