Inline Doubles truncate and round.

Inline Doubles truncate and round.
Committed: https://code.google.com/p/dart/source/detail?r=16983

[srdjan, 2012-12-15 00:59:34]

Rounding is incorrect, will fix it later.

[srdjan, 2012-12-18 21:46:06]

All tests are passing. Please have a look.

[Florian Schneider, 2013-01-08 14:24:04]

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1762: roundsd(dst, dst, Assembler::kRoundToZero);
truncate(x+0.5) does not work for some corner cases like x=0.49999999999999994
where expected result of x.round() is 0.0.

Does roundsd with mode round-to-nearest not work for us otherwise?

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32_test.cc:1324: EXPECT_EQ(-1.0, res);
Please also add the corner cases from the existing language tests here
(tests/co19/src/LibTest/core/double/round_A01_t01.dart)

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

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:4036: // Result could be any of the int
types.
Comment is not valid here.

[srdjan, 2013-01-10 22:35:52]

Please have another look. Added tests, fixed ia32 disassembler and addressed
comments.

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1762: roundsd(dst, dst, Assembler::kRoundToZero);
On 2013/01/08 14:24:04, Florian Schneider wrote:
> truncate(x+0.5) does not work for some corner cases like x=0.49999999999999994
> where expected result of x.round() is 0.0.
> 
> Does roundsd with mode round-to-nearest not work for us otherwise?

No, it doesn't. Round to nearest rounds 0.5 to 0 instead of 1. Wrote more
elaborate code (similar to V8) to handle special cases, added tests.

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32_test.cc:1324: EXPECT_EQ(-1.0, res);
On 2013/01/08 14:24:04, Florian Schneider wrote:
> Please also add the corner cases from the existing language tests here
> (tests/co19/src/LibTest/core/double/round_A01_t01.dart)
> 

Added more tests, but mainly adapted test in tests/language.

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

https://codereview.chromium.org/11573044/diff/32001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:4036: // Result could be any of the int
types.
On 2013/01/08 14:24:04, Florian Schneider wrote:
> Comment is not valid here.

Removed

[Florian Schneider, 2013-01-11 09:49:30]

LGTM with comments.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1750: // 0.5 > dst ?
For consistency with the jump label I'd rewrite the comment to

// dst < 0.5 ?

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1760: // -0.5 >= dst.
Also here:

dst <= -0.5 ?

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:1818: // 0.5 > dst ?
Same comments as on ia32.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64_...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64_...
runtime/vm/assembler_x64_test.cc:1882: EXPECT_EQ(-1.0, res);
Add the same test cases as on ia32 here as well.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1250: (recognized_kind ==
MethodRecognizer::kDoubleRound)) {
You need to check if SSE 4.1 is supported here: CPUFeatures::sse4_1_supported())

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

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:4047: // Result could be any of the int
types.
I don't understand this comment. It seems not applicable here.

https://codereview.chromium.org/11573044/diff/51001/tests/language/arithmetic...
File tests/language/arithmetic_test.dart (right):

https://codereview.chromium.org/11573044/diff/51001/tests/language/arithmetic...
tests/language/arithmetic_test.dart:40: static runOne() {
This is not directly related to your CL, but we need to make sure that all the
test cases here are actually hit in optimized state:

If this function deoptimizes repeatedly and the hard limit on the number of
deoptimizations is reached, we will never  execute the remaining optimized code
(after the last deoptimization point.

https://codereview.chromium.org/11573044/diff/51001/tests/language/arithmetic...
tests/language/arithmetic_test.dart:292: Expect.equals(-2.0, (-1.5).round());
Please also add another corner case (2^53-1) and -(2^53-1) to the test suite.


Expect.equals(9007199254740991, (9007199254740991).round());
Expect.equals(-9007199254740991, (-9007199254740991).round());

[srdjan, 2013-01-11 19:41:08]

Made code simpler ( x + 0.5) does not work for 9007199254740991. Using
roundToNearest except for 0.5 and -0.5.

Thanks for the reviews. Submitting now.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1750: // 0.5 > dst ?
On 2013/01/11 09:49:30, Florian Schneider wrote:
> For consistency with the jump label I'd rewrite the comment to
> 
> // dst < 0.5 ?

Done.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1760: // -0.5 >= dst.
On 2013/01/11 09:49:30, Florian Schneider wrote:
> Also here:
> 
> dst <= -0.5 ?

Done.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:1818: // 0.5 > dst ?
On 2013/01/11 09:49:30, Florian Schneider wrote:
> Same comments as on ia32.

Done.

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

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:4047: // Result could be any of the int
types.
On 2013/01/11 09:49:30, Florian Schneider wrote:
> I don't understand this comment. It seems not applicable here.

Sorry removed that comment, but forgot to upload.

https://codereview.chromium.org/11573044/diff/51001/tests/language/arithmetic...
File tests/language/arithmetic_test.dart (right):

https://codereview.chromium.org/11573044/diff/51001/tests/language/arithmetic...
tests/language/arithmetic_test.dart:40: static runOne() {
On 2013/01/11 09:49:30, Florian Schneider wrote:
> This is not directly related to your CL, but we need to make sure that all the
> test cases here are actually hit in optimized state:
> 
> If this function deoptimizes repeatedly and the hard limit on the number of
> deoptimizations is reached, we will never  execute the remaining optimized
code
> (after the last deoptimization point.

I agree, suggestions?

https://codereview.chromium.org/11573044/diff/51001/tests/language/arithmetic...
tests/language/arithmetic_test.dart:292: Expect.equals(-2.0, (-1.5).round());
On 2013/01/11 09:49:30, Florian Schneider wrote:
> Please also add another corner case (2^53-1) and -(2^53-1) to the test suite.
> 
> 
> Expect.equals(9007199254740991, (9007199254740991).round());
> Expect.equals(-9007199254740991, (-9007199254740991).round());
> 

Thanks, using 9007199254740991.0.

[srdjan, 2013-01-11 19:48:21]

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64_...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/assembler_x64_...
runtime/vm/assembler_x64_test.cc:1882: EXPECT_EQ(-1.0, res);
On 2013/01/11 09:49:30, Florian Schneider wrote:
> Add the same test cases as on ia32 here as well.

Done.

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11573044/diff/51001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1250: (recognized_kind ==
MethodRecognizer::kDoubleRound)) {
On 2013/01/11 09:49:30, Florian Schneider wrote:
> You need to check if SSE 4.1 is supported here:
CPUFeatures::sse4_1_supported())

Done.