[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

On ia32 return statements in C++ automatically convert signalling NaNs
to quiet NaNs, even when bit_cast is used. This CL removes all uses of
bit_cast<float> and bit_cast<double> in the wasm compiler and wasm
interpreter.

R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2639353002
Cr-Commit-Position: refs/heads/master@{#42512}

src/compiler/ia32/code-generator-ia32.cc

@@ -2106,7 +2106,7 @@ void CodeGenerator::AssembleMove(InstructionOperand* source,
       __ Move(dst, g.ToImmediate(source));
     } else if (src_constant.type() == Constant::kFloat32) {
       // TODO(turbofan): Can we do better here?
-      uint32_t src = bit_cast<uint32_t>(src_constant.ToFloat32());
+      uint32_t src = src_constant.ToFloat32AsInt();
       if (destination->IsFPRegister()) {
         XMMRegister dst = g.ToDoubleRegister(destination);
         __ Move(dst, src);
@@ -2117,7 +2117,7 @@ void CodeGenerator::AssembleMove(InstructionOperand* source,
       }
     } else {
       DCHECK_EQ(Constant::kFloat64, src_constant.type());
-      uint64_t src = bit_cast<uint64_t>(src_constant.ToFloat64());
+      uint64_t src = src_constant.ToFloat64AsInt();
       uint32_t lower = static_cast<uint32_t>(src);
       uint32_t upper = static_cast<uint32_t>(src >> 32);
       if (destination->IsFPRegister()) {

src/compiler/instruction.h

@@ -1065,16 +1065,33 @@ class V8_EXPORT_PRIVATE Constant final {
   }
 
   float ToFloat32() const {
+    // TODO(ahaas): We should remove this function. If value_ has the bit
+    // representation of a signalling NaN, then returning it as float can cause
+    // the signalling bit to flip, and value_ is returned as a quiet NaN.
     DCHECK_EQ(kFloat32, type());
     return bit_cast<float>(static_cast<int32_t>(value_));
   }
 
+  uint32_t ToFloat32AsInt() const {
+    DCHECK_EQ(kFloat32, type());
+    return bit_cast<uint32_t>(static_cast<int32_t>(value_));
+  }
+
   double ToFloat64() const {
+    // TODO(ahaas): We should remove this function. If value_ has the bit
+    // representation of a signalling NaN, then returning it as float can cause
+    // the signalling bit to flip, and value_ is returned as a quiet NaN.
     if (type() == kInt32) return ToInt32();
     DCHECK_EQ(kFloat64, type());
     return bit_cast<double>(value_);
   }
 
+  uint64_t ToFloat64AsInt() const {
+    if (type() == kInt32) return ToInt32();
+    DCHECK_EQ(kFloat64, type());
+    return bit_cast<uint64_t>(value_);
+  }
+
   ExternalReference ToExternalReference() const {
     DCHECK_EQ(kExternalReference, type());
     return bit_cast<ExternalReference>(static_cast<intptr_t>(value_));

src/wasm/function-body-decoder.h

@@ -70,7 +70,9 @@ struct ImmF32Operand {
   float value;
   unsigned length;
   inline ImmF32Operand(Decoder* decoder, const byte* pc) {
-    value = bit_cast<float>(decoder->checked_read_u32(pc, 1, "immf32"));
+    // Avoid bit_cast because it might not preserve the signalling bit of a NaN.
+    uint32_t tmp = decoder->checked_read_u32(pc, 1, "immf32");
+    value = *reinterpret_cast<float*>(&tmp);
     length = 4;
   }
 };
@@ -79,7 +81,9 @@ struct ImmF64Operand {
   double value;
   unsigned length;
   inline ImmF64Operand(Decoder* decoder, const byte* pc) {
-    value = bit_cast<double>(decoder->checked_read_u64(pc, 1, "immf64"));
+    // Avoid bit_cast because it might not preserve the signalling bit of a NaN.
+    uint64_t tmp = decoder->checked_read_u64(pc, 1, "immf64");
+    value = *reinterpret_cast<double*>(&tmp);
     length = 8;
   }
 };

src/wasm/wasm-interpreter.cc

@@ -159,8 +159,6 @@ namespace wasm {
   V(F64UConvertI64, uint64_t)    \
   V(F64ConvertF32, float)        \
   V(F64ReinterpretI64, int64_t)  \
-  V(I32ReinterpretF32, float)    \
-  V(I64ReinterpretF64, double)   \
   V(I32AsmjsSConvertF32, float)  \
   V(I32AsmjsUConvertF32, float)  \
   V(I32AsmjsSConvertF64, double) \
@@ -606,12 +604,12 @@ static inline double ExecuteF64ReinterpretI64(int64_t a, TrapReason* trap) {
   return bit_cast<double>(a);
 }
 
-static inline int32_t ExecuteI32ReinterpretF32(float a, TrapReason* trap) {
-  return bit_cast<int32_t>(a);
+static inline int32_t ExecuteI32ReinterpretF32(WasmVal a) {
+  return a.to_unchecked<int32_t>();
 }
 
-static inline int64_t ExecuteI64ReinterpretF64(double a, TrapReason* trap) {
-  return bit_cast<int64_t>(a);
+static inline int64_t ExecuteI64ReinterpretF64(WasmVal a) {
+  return a.to_unchecked<int64_t>();
 }
 
 static inline int32_t ExecuteGrowMemory(uint32_t delta_pages,
@@ -1548,6 +1546,19 @@ class ThreadImpl {
           len = 1 + operand.length;
           break;
         }
+        // We need to treat kExprI32ReinterpretF32 and kExprI64ReinterpretF64
+        // specially to guarantee that the quiet bit of a NaN is preserved on
+        // ia32 by the reinterpret casts.
+        case kExprI32ReinterpretF32: {
+          WasmVal result(ExecuteI32ReinterpretF32(Pop()));
+          Push(pc, result);
+          break;
+        }
+        case kExprI64ReinterpretF64: {
+          WasmVal result(ExecuteI64ReinterpretF64(Pop()));
+          Push(pc, result);
+          break;
+        }
 #define EXECUTE_SIMPLE_BINOP(name, ctype, op)             \
   case kExpr##name: {                                     \
     WasmVal rval = Pop();                                 \

src/wasm/wasm-interpreter.h

@@ -56,12 +56,21 @@ struct WasmVal {
 #undef DECLARE_CONSTRUCTOR
 
   template <typename T>
-  T to() {
+  inline T to() {
+    UNREACHABLE();
+  }
+
+  template <typename T>
+  inline T to_unchecked() {
     UNREACHABLE();
   }
 };
 
 #define DECLARE_CAST(field, localtype, ctype) \
+  template <>                                 \
+  inline ctype WasmVal::to_unchecked() {      \
+    return val.field;                         \
+  }                                           \
   template <>                                 \
   inline ctype WasmVal::to() {                \
     CHECK_EQ(localtype, type);                \
@@ -70,11 +79,6 @@ struct WasmVal {
 FOREACH_UNION_MEMBER(DECLARE_CAST)
 #undef DECLARE_CAST
 
-template <>
-inline void WasmVal::to() {
-  CHECK_EQ(kWasmStmt, type);
-}
-
 // Representation of frames within the interpreter.
 class WasmFrame {
  public:

test/cctest/wasm/test-run-wasm-64.cc

@@ -1319,6 +1319,16 @@ WASM_EXEC_TEST(I64ReinterpretF64) {
   }
 }
 
+WASM_EXEC_TEST(SignallingNanSurvivesI64ReinterpretF64) {
+  REQUIRE(I64ReinterpretF64);
+  WasmRunner<int64_t> r(execution_mode);
+  BUILD(r, WASM_I64_REINTERPRET_F64(WASM_SEQ(kExprF64Const, 0x00, 0x00, 0x00,
+                                             0x00, 0x00, 0x00, 0xf4, 0x7f)));
+
+  // This is a signalling nan.
+  CHECK_EQ(0x7ff4000000000000, r.Call());
+}
+
 WASM_EXEC_TEST(F64ReinterpretI64) {
   REQUIRE(F64ReinterpretI64);
   WasmRunner<int64_t, int64_t> r(execution_mode);

test/cctest/wasm/test-run-wasm.cc

@@ -1048,6 +1048,16 @@ WASM_EXEC_TEST(I32ReinterpretF32) {
   }
 }
 
+WASM_EXEC_TEST(SignallingNanSurvivesI32ReinterpretF32) {
+  WasmRunner<int32_t> r(execution_mode);
+
+  BUILD(r, WASM_I32_REINTERPRET_F32(
+               WASM_SEQ(kExprF32Const, 0x00, 0x00, 0xa0, 0x7f)));
+
+  // This is a signalling nan.
+  CHECK_EQ(0x7fa00000, r.Call());
+}
+
 WASM_EXEC_TEST_WITH_TRAP(LoadMaxUint32Offset) {
   WasmRunner<int32_t> r(execution_mode);
   r.module().AddMemoryElems<int32_t>(8);