Avoid just truncating to the nearby int in double.hashCode.

Preserves the property that doubles and ints that are equal
share the same hash code, but gives different doubles a much
higher chance of having different hash codes.

BUG=#2884

Change-Id: Ia97164087db568f26ffbbd6854ed9c19f81e9611
Reviewed-on: https://dart-review.googlesource.com/15140
Commit-Queue: Kasper Lund <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>

Author: Kasper Lund

runtime/lib/double.cc

@@ -118,6 +118,31 @@ static RawInteger* DoubleToInteger(double val, const char* error_msg) {
   return result.AsValidInteger();
 }
 
+DEFINE_NATIVE_ENTRY(Double_hashCode, 1) {
+  double val = Double::CheckedHandle(arguments->NativeArgAt(0)).value();
+  if (FLAG_trace_intrinsified_natives) {
+    OS::Print("Double_hashCode %f\n", val);
+  }
+  if (val >= static_cast<double>(kMinInt64) &&
+      val <= static_cast<double>(kMaxInt64)) {
+    int64_t ival = static_cast<int64_t>(val);
+    if (static_cast<double>(ival) == val) {
+      return Integer::New(ival);
+    }
+  } else if (!FLAG_limit_ints_to_64_bits && !isinf(val) && !isnan(val)) {
+    // Since this code is temporary until we limit ints to 64 bits, we
+    // reuse the existing DoubleToInteger helper function and pass it
+    // an empty error message ("") because it cannot fail.
+    const Integer& bigint = Integer::Handle(DoubleToInteger(val, ""));
+    if (bigint.AsDoubleValue() == val) {
+      return bigint.raw();
+    }
+  }
+
+  uint64_t uval = bit_cast<uint64_t>(val);
+  return Smi::New(((uval >> 32) ^ (uval)) & kSmiMax);
+}
+
 DEFINE_NATIVE_ENTRY(Double_trunc_div, 2) {
   double left = Double::CheckedHandle(arguments->NativeArgAt(0)).value();
   GET_NON_NULL_NATIVE_ARGUMENT(Double, right_object, arguments->NativeArgAt(1));

runtime/lib/double.dart

@@ -7,11 +7,8 @@
 class _Double implements double {
   factory _Double.fromInteger(int value) native "Double_doubleFromInteger";
 
-  // TODO: Make a stared static method for hashCode and _identityHashCode
-  //       when semantics are corrected as described in:
-  //       https://github.com/dart-lang/sdk/issues/2884
-  int get hashCode => (isNaN || isInfinite) ? 0 : toInt();
-  int get _identityHashCode => (isNaN || isInfinite) ? 0 : toInt();
+  int get hashCode native "Double_hashCode";
+  int get _identityHashCode native "Double_hashCode";
 
   double operator +(num other) {
     return _add(other.toDouble());

runtime/vm/bootstrap_natives.h

@@ -77,6 +77,7 @@ namespace dart {
   V(Developer_log, 8)                                                          \
   V(Developer_postEvent, 2)                                                    \
   V(Developer_webServerControl, 2)                                             \
+  V(Double_hashCode, 1)                                                        \
   V(Double_getIsNegative, 1)                                                   \
   V(Double_getIsInfinite, 1)                                                   \
   V(Double_getIsNaN, 1)                                                        \

runtime/vm/compiler/intrinsifier.cc

@@ -1147,6 +1147,10 @@ bool Intrinsifier::Build_DoubleRound(FlowGraph* flow_graph) {
   return BuildInvokeMathCFunction(&builder, MethodRecognizer::kDoubleRound);
 }
 
+void Intrinsifier::Double_identityHash(Assembler* assembler) {
+  Double_hashCode(assembler);
+}
+
 void Intrinsifier::RegExp_ExecuteMatch(Assembler* assembler) {
   IntrinsifyRegExpExecuteMatch(assembler, /*sticky=*/false);
 }

runtime/vm/compiler/intrinsifier_arm.cc

@@ -1444,6 +1444,53 @@ void Intrinsifier::DoubleToInteger(Assembler* assembler) {
   }
 }
 
+void Intrinsifier::Double_hashCode(Assembler* assembler) {
+  // TODO(dartbug.com/31174): Convert this to a graph intrinsic.
+
+  if (!TargetCPUFeatures::vfp_supported()) return;
+
+  // Load double value and check that it isn't NaN, since ARM gives an
+  // FPU exception if you try to convert NaN to an int.
+  Label double_hash;
+  __ ldr(R1, Address(SP, 0 * kWordSize));
+  __ LoadDFromOffset(D0, R1, Double::value_offset() - kHeapObjectTag);
+  __ vcmpd(D0, D0);
+  __ vmstat();
+  __ b(&double_hash, VS);
+
+  // Convert double value to signed 32-bit int in R0.
+  __ vcvtid(S2, D0);
+  __ vmovrs(R0, S2);
+
+  // Tag the int as a Smi, making sure that it fits; this checks for
+  // overflow in the conversion from double to int. Conversion
+  // overflow is signalled by vcvt through clamping R0 to either
+  // INT32_MAX or INT32_MIN (saturation).
+  Label fall_through;
+  ASSERT(kSmiTag == 0 && kSmiTagShift == 1);
+  __ adds(R0, R0, Operand(R0));
+  __ b(&fall_through, VS);
+
+  // Compare the two double values. If they are equal, we return the
+  // Smi tagged result immediately as the hash code.
+  __ vcvtdi(D1, S2);
+  __ vcmpd(D0, D1);
+  __ vmstat();
+  __ bx(LR, EQ);
+
+  // Convert the double bits to a hash code that fits in a Smi.
+  __ Bind(&double_hash);
+  __ ldr(R0, FieldAddress(R1, Double::value_offset()));
+  __ ldr(R1, FieldAddress(R1, Double::value_offset() + 4));
+  __ eor(R0, R0, Operand(R1));
+  __ AndImmediate(R0, R0, kSmiMax);
+  __ SmiTag(R0);
+  __ Ret();
+
+  // Fall into the native C++ implementation.
+  __ Bind(&fall_through);
+}
+
 void Intrinsifier::MathSqrt(Assembler* assembler) {
   if (TargetCPUFeatures::vfp_supported()) {
     Label fall_through, is_smi, double_op;

runtime/vm/compiler/intrinsifier_arm64.cc

@@ -1522,6 +1522,49 @@ void Intrinsifier::DoubleToInteger(Assembler* assembler) {
   __ Bind(&fall_through);
 }
 
+void Intrinsifier::Double_hashCode(Assembler* assembler) {
+  // TODO(dartbug.com/31174): Convert this to a graph intrinsic.
+
+  // Load double value and check that it isn't NaN, since ARM gives an
+  // FPU exception if you try to convert NaN to an int.
+  Label double_hash;
+  __ ldr(R1, Address(SP, 0 * kWordSize));
+  __ LoadDFieldFromOffset(V0, R1, Double::value_offset());
+  __ fcmpd(V0, V0);
+  __ b(&double_hash, VS);
+
+  // Convert double value to signed 64-bit int in R0 and back to a
+  // double value in V1.
+  __ fcvtzds(R0, V0);
+  __ scvtfdx(V1, R0);
+
+  // Tag the int as a Smi, making sure that it fits; this checks for
+  // overflow in the conversion from double to int. Conversion
+  // overflow is signalled by fcvt through clamping R0 to either
+  // INT64_MAX or INT64_MIN (saturation).
+  Label fall_through;
+  ASSERT(kSmiTag == 0 && kSmiTagShift == 1);
+  __ adds(R0, R0, Operand(R0));
+  __ b(&fall_through, VS);
+
+  // Compare the two double values. If they are equal, we return the
+  // Smi tagged result immediately as the hash code.
+  __ fcmpd(V0, V1);
+  __ b(&double_hash, NE);
+  __ ret();
+
+  // Convert the double bits to a hash code that fits in a Smi.
+  __ Bind(&double_hash);
+  __ fmovrd(R0, V0);
+  __ eor(R0, R0, Operand(R0, LSR, 32));
+  __ AndImmediate(R0, R0, kSmiMax);
+  __ SmiTag(R0);
+  __ ret();
+
+  // Fall into the native C++ implementation.
+  __ Bind(&fall_through);
+}
+
 void Intrinsifier::MathSqrt(Assembler* assembler) {
   Label fall_through, is_smi, double_op;
   TestLastArgumentIsDouble(assembler, &is_smi, &fall_through);

runtime/vm/compiler/intrinsifier_ia32.cc

@@ -1558,6 +1558,44 @@ void Intrinsifier::DoubleToInteger(Assembler* assembler) {
   __ Bind(&fall_through);
 }
 
+void Intrinsifier::Double_hashCode(Assembler* assembler) {
+  // TODO(dartbug.com/31174): Convert this to a graph intrinsic.
+
+  // Convert double value to signed 32-bit int in EAX and
+  // back to a double in XMM1.
+  __ movl(ECX, Address(ESP, +1 * kWordSize));
+  __ movsd(XMM0, FieldAddress(ECX, Double::value_offset()));
+  __ cvttsd2si(EAX, XMM0);
+  __ cvtsi2sd(XMM1, EAX);
+
+  // Tag the int as a Smi, making sure that it fits; this checks for
+  // overflow and NaN in the conversion from double to int. Conversion
+  // overflow from cvttsd2si is signalled with an INT32_MIN value.
+  Label fall_through;
+  ASSERT(kSmiTag == 0 && kSmiTagShift == 1);
+  __ addl(EAX, EAX);
+  __ j(OVERFLOW, &fall_through, Assembler::kNearJump);
+
+  // Compare the two double values. If they are equal, we return the
+  // Smi tagged result immediately as the hash code.
+  Label double_hash;
+  __ comisd(XMM0, XMM1);
+  __ j(NOT_EQUAL, &double_hash, Assembler::kNearJump);
+  __ ret();
+
+  // Convert the double bits to a hash code that fits in a Smi.
+  __ Bind(&double_hash);
+  __ movl(EAX, FieldAddress(ECX, Double::value_offset()));
+  __ movl(ECX, FieldAddress(ECX, Double::value_offset() + 4));
+  __ xorl(EAX, ECX);
+  __ andl(EAX, Immediate(kSmiMax));
+  __ SmiTag(EAX);
+  __ ret();
+
+  // Fall into the native C++ implementation.
+  __ Bind(&fall_through);
+}
+
 // Argument type is not known
 void Intrinsifier::MathSqrt(Assembler* assembler) {
   Label fall_through, is_smi, double_op;

runtime/vm/compiler/intrinsifier_x64.cc

@@ -1428,6 +1428,45 @@ void Intrinsifier::DoubleToInteger(Assembler* assembler) {
   __ Bind(&fall_through);
 }
 
+void Intrinsifier::Double_hashCode(Assembler* assembler) {
+  // TODO(dartbug.com/31174): Convert this to a graph intrinsic.
+
+  // Convert double value to signed 64-bit int in RAX and
+  // back to a double in XMM1.
+  __ movq(RCX, Address(RSP, +1 * kWordSize));
+  __ movsd(XMM0, FieldAddress(RCX, Double::value_offset()));
+  __ cvttsd2siq(RAX, XMM0);
+  __ cvtsi2sdq(XMM1, RAX);
+
+  // Tag the int as a Smi, making sure that it fits; this checks for
+  // overflow and NaN in the conversion from double to int. Conversion
+  // overflow from cvttsd2si is signalled with an INT64_MIN value.
+  Label fall_through;
+  ASSERT(kSmiTag == 0 && kSmiTagShift == 1);
+  __ addq(RAX, RAX);
+  __ j(OVERFLOW, &fall_through, Assembler::kNearJump);
+
+  // Compare the two double values. If they are equal, we return the
+  // Smi tagged result immediately as the hash code.
+  Label double_hash;
+  __ comisd(XMM0, XMM1);
+  __ j(NOT_EQUAL, &double_hash, Assembler::kNearJump);
+  __ ret();
+
+  // Convert the double bits to a hash code that fits in a Smi.
+  __ Bind(&double_hash);
+  __ movq(RAX, FieldAddress(RCX, Double::value_offset()));
+  __ movq(RCX, RAX);
+  __ shrq(RCX, Immediate(32));
+  __ xorq(RAX, RCX);
+  __ andq(RAX, Immediate(kSmiMax));
+  __ SmiTag(RAX);
+  __ ret();
+
+  // Fall into the native C++ implementation.
+  __ Bind(&fall_through);
+}
+
 void Intrinsifier::MathSqrt(Assembler* assembler) {
   Label fall_through, is_smi, double_op;
   TestLastArgumentIsDouble(assembler, &is_smi, &fall_through);

runtime/vm/compiler/method_recognizer.h

@@ -161,6 +161,8 @@ namespace dart {
   V(_Double, -, Double_sub, Double, 0x76768546)                                \
   V(_Double, *, Double_mul, Double, 0x66c66e3d)                                \
   V(_Double, /, Double_div, Double, 0x034b9f08)                                \
+  V(_Double, get:hashCode, Double_hashCode, Dynamic, 0x702b0358)               \
+  V(_Double, get:_identityHashCode, Double_identityHash, Dynamic, 0x7bd9e0ea)  \
   V(_Double, get:isNaN, Double_getIsNaN, Bool, 0x0af9604a)                     \
   V(_Double, get:isInfinite, Double_getIsInfinite, Bool, 0x0f7a56e8)           \
   V(_Double, get:isNegative, Double_getIsNegative, Bool, 0x3a597395)           \
@@ -228,8 +230,7 @@ namespace dart {
   V(_IntegerImplementation, >=, Integer_greaterEqualThan, Bool, 0x57491a62)    \
   V(_IntegerImplementation, <<, Integer_shl, Dynamic, 0x1050c9a8)              \
   V(_IntegerImplementation, >>, Integer_sar, Dynamic, 0x39af1c69)              \
-  V(_Double, toInt, DoubleToInteger, Dynamic, 0x26ef344b)
-
+  V(_Double, toInt, DoubleToInteger, Dynamic, 0x26ef344b)                      \
 
 #define MATH_LIB_INTRINSIC_LIST(V)                                             \
   V(::, sqrt, MathSqrt, Double, 0x70482cf3)                                    \

runtime/vm/simulator_arm64.cc

@@ -3125,7 +3125,13 @@ void Simulator::DecodeFPIntCvt(Instr* instr) {
     } else if (instr->Bits(16, 5) == 24) {
       // Format(instr, "fcvtzds'sf 'rd, 'vn");
       const double vn_val = bit_cast<double, int64_t>(get_vregisterd(vn, 0));
-      set_register(instr, rd, static_cast<int64_t>(vn_val), instr->RdMode());
+      if (vn_val >= static_cast<double>(INT64_MAX)) {
+        set_register(instr, rd, INT64_MAX, instr->RdMode());
+      } else if (vn_val <= static_cast<double>(INT64_MIN)) {
+        set_register(instr, rd, INT64_MIN, instr->RdMode());
+      } else {
+        set_register(instr, rd, static_cast<int64_t>(vn_val), instr->RdMode());
+      }
     } else {
       UnimplementedInstruction(instr);
     }

tests/corelib_2/double_hash_code_test.dart

@@ -0,0 +1,23 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+
+main() {
+  for (int x in [0, 1, 0xffff, 0xffffffff, 0x111111111111, 0xffffffffffff]) {
+    test(x);
+    test(-x);
+  }
+
+  // Test with ints outside the 53-bit range that are known to have an
+  // exact double representation.
+  test(9007199254840856);
+  test(144115188075954880);
+  test(936748722493162112);
+}
+
+test(int x) {
+  Expect.equals(x, x.toDouble().toInt(), "bad test argument ($x)");
+  Expect.equals(x.hashCode, x.toDouble().hashCode);
+}

tests/standalone_2/double_hash_distribution_test.dart

@@ -0,0 +1,45 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Test that the distribution of hash codes for doubles is reasonable.
+
+import 'package:expect/expect.dart';
+
+main() {
+  Expect.isTrue(ratio(0, 1) >= 0.95);
+  Expect.isTrue(ratio(0, 100) >= 0.95);
+  Expect.isTrue(ratio(0, 0xffffff) >= 0.95);
+  Expect.isTrue(ratio(0xffffff) >= 0.95);
+  Expect.isTrue(ratio(0xffffffff) >= 0.95);
+  Expect.isTrue(ratio(0xffffffffffffff) >= 0.95);
+
+  Expect.isTrue(ratio(0, -1) >= 0.95);
+  Expect.isTrue(ratio(0, -100) >= 0.95);
+  Expect.isTrue(ratio(0, -0xffffff) >= 0.95);
+  Expect.isTrue(ratio(-0xffffff) >= 0.95);
+  Expect.isTrue(ratio(-0xffffffff) >= 0.95);
+  Expect.isTrue(ratio(-0xffffffffffffff) >= 0.95);
+}
+
+double ratio(num start, [num end]) {
+  final n = 1000;
+  end ??= (start + 1) * 2;
+
+  // Collect the set of distinct doubles and the
+  // set of distinct hash codes.
+  final doubles = new Set<double>();
+  final codes = new Set<int>();
+
+  final step = (end.toDouble() - start.toDouble()) / n;
+  var current = start.toDouble();
+  for (int i = 0; i < n; i++) {
+    doubles.add(current);
+    codes.add(current.hashCode);
+    current += step;
+  }
+
+  // Return the ratio between distinct doubles and
+  // distinct hash codes.
+  return codes.length / doubles.length;
+}