Fix 23563: double unary- operator unstable for NANs

Fix 23563: double unary- operator unstable for NANs

BUG=
R=koda@google.com

Committed: https://github.com/dart-lang/sdk/commit/c0824e77255522a5f3e981b0ea88f7ccee60259a

[srdjan, 2015-06-02 20:43:48]

srdjan@google.com changed reviewers:
+ koda@google.com

[koda, 2015-06-02 21:20:57]

https://codereview.chromium.org/1160453003/diff/40001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/1160453003/diff/40001/runtime/lib/double.cc#n...
runtime/lib/double.cc:309: res = NAN;
Here, you are also normalizing to a particular NAN constant. I was under the
impression that it should flip the sign bit of the input itself and return that.
Please clarify.

Also, you are assuming a particular behavior from the C++ compiler. Why not
simply reinterpret_cast and then xor with the proper bit pattern?

https://codereview.chromium.org/1160453003/diff/40001/tests/language/nan_iden...
File tests/language/nan_identical_test.dart (right):

https://codereview.chromium.org/1160453003/diff/40001/tests/language/nan_iden...
tests/language/nan_identical_test.dart:13:
Expect.isTrue(checkIdentical(-double.NAN, -double.NAN));
You should also test the return value for other NaNs.

[koda, 2015-06-02 21:27:36]

On 2015/06/02 21:20:57, koda wrote:
> https://codereview.chromium.org/1160453003/diff/40001/runtime/lib/double.cc
> File runtime/lib/double.cc (right):
> 
>
https://codereview.chromium.org/1160453003/diff/40001/runtime/lib/double.cc#n...
> runtime/lib/double.cc:309: res = NAN;
> Here, you are also normalizing to a particular NAN constant. I was under the
> impression that it should flip the sign bit of the input itself and return
that.
> Please clarify.
> 
> Also, you are assuming a particular behavior from the C++ compiler. Why not
> simply reinterpret_cast and then xor with the proper bit pattern?
> 
>
https://codereview.chromium.org/1160453003/diff/40001/tests/language/nan_iden...
> File tests/language/nan_identical_test.dart (right):
> 
>
https://codereview.chromium.org/1160453003/diff/40001/tests/language/nan_iden...
> tests/language/nan_identical_test.dart:13:
> Expect.isTrue(checkIdentical(-double.NAN, -double.NAN));
> You should also test the return value for other NaNs.

Here's an example of how to create other NaNs:
https://code.google.com/p/dart/issues/detail?id=18738

[srdjan, 2015-06-02 22:32:11]

Thanks, PTAL.

https://codereview.chromium.org/1160453003/diff/40001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/1160453003/diff/40001/runtime/lib/double.cc#n...
runtime/lib/double.cc:309: res = NAN;
On 2015/06/02 21:20:57, koda wrote:
> Here, you are also normalizing to a particular NAN constant. I was under the
> impression that it should flip the sign bit of the input itself and return
that.
> Please clarify.
> 
> Also, you are assuming a particular behavior from the C++ compiler. Why not
> simply reinterpret_cast and then xor with the proper bit pattern?

Renamed method to Double_flipSignBit. Flipping sign bit of any double,
simplified the double's unary- code.

https://codereview.chromium.org/1160453003/diff/40001/tests/language/nan_iden...
File tests/language/nan_identical_test.dart (right):

https://codereview.chromium.org/1160453003/diff/40001/tests/language/nan_iden...
tests/language/nan_identical_test.dart:13:
Expect.isTrue(checkIdentical(-double.NAN, -double.NAN));
On 2015/06/02 21:20:57, koda wrote:
> You should also test the return value for other NaNs.

Done.

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.dart
File runtime/lib/double.dart (right):

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.dar...
runtime/lib/double.dart:51: return this._flipSignBit;
Calling out to native should not be a performance problem as the optimized code
inlines double unary- to best assembly pattern.

[koda, 2015-06-02 23:41:02]

LGTM if you add the -(-x) test.

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.cc#...
runtime/lib/double.cc:308: *(reinterpret_cast<const int64_t*>(&in_val)) ^
0x8000000000000000LL;
Just curious: I wonder if there is any spec/de-facto difference between doing
reinterpret_cast on pointers to int64/double vs. doing it directly on the
values? If either works, the latter seems simpler, but I don't feel strongly
about it.

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.cc#...
runtime/lib/double.cc:308: *(reinterpret_cast<const int64_t*>(&in_val)) ^
0x8000000000000000LL;
Consider putting the constant in globals.h. Also, do all compilers accept this
syntax for a 64-bit literal?

https://codereview.chromium.org/1160453003/diff/120001/tests/language/nan_ide...
File tests/language/nan_identical_test.dart (right):

https://codereview.chromium.org/1160453003/diff/120001/tests/language/nan_ide...
tests/language/nan_identical_test.dart:45: }
Please also check that -(-x) is identical to x for all x in {a,b}

[srdjan, 2015-06-03 00:02:52]

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.cc#...
runtime/lib/double.cc:308: *(reinterpret_cast<const int64_t*>(&in_val)) ^
0x8000000000000000LL;
On 2015/06/02 23:41:02, koda wrote:
> Just curious: I wonder if there is any spec/de-facto difference between doing
> reinterpret_cast on pointers to int64/double vs. doing it directly on the
> values? If either works, the latter seems simpler, but I don't feel strongly
> about it.

As far as I remember, I had problems with doing it on values (some compilers
complained) and had to do it on pointers instead.

https://codereview.chromium.org/1160453003/diff/120001/runtime/lib/double.cc#...
runtime/lib/double.cc:308: *(reinterpret_cast<const int64_t*>(&in_val)) ^
0x8000000000000000LL;
On 2015/06/02 23:41:02, koda wrote:
> Consider putting the constant in globals.h. Also, do all compilers accept this
> syntax for a 64-bit literal?

AFAIK yes, we are using it in simd28.cc and assembler_{ia32,x64}.cc. I think it
is part of standard.
http://gcc.gnu.org/onlinedocs/gcc/Long-Long.html

Just in case, added to globals.h:
const int64_t kSignBitDouble = DART_INT64_C(0x8000000000000000);

https://codereview.chromium.org/1160453003/diff/120001/tests/language/nan_ide...
File tests/language/nan_identical_test.dart (right):

https://codereview.chromium.org/1160453003/diff/120001/tests/language/nan_ide...
tests/language/nan_identical_test.dart:45: }
On 2015/06/02 23:41:02, koda wrote:
> Please also check that -(-x) is identical to x for all x in {a,b}

Done.

[Anders Johnsen, 2015-06-03 06:37:36]

ajohnsen@google.com changed reviewers:
+ ajohnsen@google.com

[Anders Johnsen, 2015-06-03 06:37:37]

DBC

https://codereview.chromium.org/1160453003/diff/170001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/1160453003/diff/170001/runtime/lib/double.cc#...
runtime/lib/double.cc:310: return Double::New(out_val);
I may have missed out here, but my experience tells me that 

  -in_val

should do it. Is that not the case?

[koda, 2015-06-03 17:28:11]

https://codereview.chromium.org/1160453003/diff/170001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/1160453003/diff/170001/runtime/lib/double.cc#...
runtime/lib/double.cc:310: return Double::New(out_val);
On 2015/06/03 06:37:36, Anders Johnsen wrote:
> I may have missed out here, but my experience tells me that 
> 
>   -in_val
> 
> should do it. Is that not the case?

Even if the C++ standard would mandate the behavior we want (does it?), and all
reasonable compilers correctly implement it (do they?), I still think being
explicit about the exact semantics is preferable, and any performance loss is
noise compared to the runtime call overhead.

[srdjan, 2015-06-03 17:56:40]

On 2015/06/03 17:28:11, koda wrote:
> https://codereview.chromium.org/1160453003/diff/170001/runtime/lib/double.cc
> File runtime/lib/double.cc (right):
> 
>
https://codereview.chromium.org/1160453003/diff/170001/runtime/lib/double.cc#...
> runtime/lib/double.cc:310: return Double::New(out_val);
> On 2015/06/03 06:37:36, Anders Johnsen wrote:
> > I may have missed out here, but my experience tells me that 
> > 
> >   -in_val
> > 
> > should do it. Is that not the case?
> 
> Even if the C++ standard would mandate the behavior we want (does it?), and
all
> reasonable compilers correctly implement it (do they?), I still think being
> explicit about the exact semantics is preferable, and any performance loss is
> noise compared to the runtime call overhead.

I agree with Daniel. Submitting as it is.

[srdjan, 2015-06-03 18:00:39]

Committed patchset #12 (id:210001) manually as
c0824e77255522a5f3e981b0ea88f7ccee60259a (presubmit successful).