Interpreter: Don't change NaN bits when dividing by 1 (#2958)

It's valid to change NaN bits in that case per the wasm spec, but
if we do so then fuzz testcases will fail on the optimization of

nan:foo / 1 => nan:foo

That is, it is ok to leave the bits as they are, and if we do that then
we are consistent with the simple and valid optimization of removing
a divide by 1.

Found by the fuzzer - looks like on x64 on some float32 NaNs,
the bits will actually change (see the testcase). I've seen this on
two machines consistently, so it's normal apparently.

Disable an old wasm spectest that has been updated in upstream
anyhow, but the new test here is even more strict and verifies
the interpreter literally changes no bits.

4 files changed, 54 insertions, 2 deletions

diff --git a/src/wasm/literal.cpp b/src/wasm/literal.cpp
index 29d047723..f0a065c45 100644
--- a/src/wasm/literal.cpp
+++ b/src/wasm/literal.cpp
@@ -868,6 +868,18 @@ Literal Literal::div(const Literal& other) const {
         case FP_INFINITE: // fallthrough
         case FP_NORMAL:   // fallthrough
         case FP_SUBNORMAL:
+          // Special-case division by 1. nan / 1 can change nan bits per the
+          // wasm spec, but it is ok to just return that original nan, and we
+          // do that here so that we are consistent with the optimization of
+          // removing the / 1 and leaving just the nan. That is, if we just
+          // do a normal divide and the CPU decides to change the bits, we'd
+          // give a different result on optimized code, which would look like
+          // it was a bad optimization. So out of all the valid results to
+          // return here, return the simplest one that is consistent with
+          // optimization.
+          if (rhs == 1) {
+            return Literal(lhs);
+          }
           return Literal(lhs / rhs);
         default:
           WASM_UNREACHABLE("invalid fp classification");
@@ -896,6 +908,10 @@ Literal Literal::div(const Literal& other) const {
         case FP_INFINITE: // fallthrough
         case FP_NORMAL:   // fallthrough
         case FP_SUBNORMAL:
+          // See above comment on f32.
+          if (rhs == 1) {
+            return Literal(lhs);
+          }
           return Literal(lhs / rhs);
         default:
           WASM_UNREACHABLE("invalid fp classification");
diff --git a/test/passes/fuzz-exec_O.txt b/test/passes/fuzz-exec_O.txt
index 841507f60..9c8280586 100644
--- a/test/passes/fuzz-exec_O.txt
+++ b/test/passes/fuzz-exec_O.txt
@@ -30,3 +30,22 @@
 [trap final > memory: 18446744073709551615 > 65514]
 [fuzz-exec] comparing func_0
 [fuzz-exec] comparing func_1
+[fuzz-exec] calling func_113
+[LoggingExternalInterface logging -nan:0x23017a]
+[fuzz-exec] note result: func_113 => 113
+(module
+ (type $f32_=>_none (func (param f32)))
+ (type $none_=>_i64 (func (result i64)))
+ (import "fuzzing-support" "log-f32" (func $fimport$0 (param f32)))
+ (export "func_113" (func $0))
+ (func $0 (; has Stack IR ;) (result i64)
+  (call $fimport$0
+   (f32.const -nan:0x23017a)
+  )
+  (i64.const 113)
+ )
+)
+[fuzz-exec] calling func_113
+[LoggingExternalInterface logging -nan:0x23017a]
+[fuzz-exec] note result: func_113 => 113
+[fuzz-exec] comparing func_113
diff --git a/test/passes/fuzz-exec_O.wast b/test/passes/fuzz-exec_O.wast
index 3d03de714..44bbd7101 100644
--- a/test/passes/fuzz-exec_O.wast
+++ b/test/passes/fuzz-exec_O.wast
@@ -20,4 +20,19 @@
   )
  )
 )
+(module
+ (type $f32_=>_none (func (param f32)))
+ (type $none_=>_i64 (func (result i64)))
+ (import "fuzzing-support" "log-f32" (func $fimport$0 (param f32)))
+ (export "func_113" (func $0))
+ (func $0 (result i64)
+  (call $fimport$0
+   (f32.div
+    (f32.const -nan:0x23017a) ;; div by 1 can be removed, leaving this nan
+    (f32.const 1)             ;; as it is. wasm semantics allow nan bits to
+   )                          ;; change, but the interpreter should not do so,
+  )                           ;; so that it does not fail on that opt.
+  (i64.const 113)
+ )
+)
 
diff --git a/test/spec/old_float_exprs.wast b/test/spec/old_float_exprs.wast
index 7900832b0..44515a32e 100644
--- a/test/spec/old_float_exprs.wast
+++ b/test/spec/old_float_exprs.wast
@@ -133,8 +133,10 @@
     (f64.div (local.get $x) (f64.const 1.0)))
 )
 
-(assert_return (invoke "f32.no_fold_div_one" (f32.const nan:0x200000)) (f32.const nan:0x600000))
-(assert_return (invoke "f64.no_fold_div_one" (f64.const nan:0x4000000000000)) (f64.const nan:0xc000000000000))
+;; XXX BINARYEN: disable this test, as we have testing for the more strict property
+;;               of not changing the bits at all in our interpreter
+;; (assert_return (invoke "f32.no_fold_div_one" (f32.const nan:0x200000)) (f32.const nan:arithmetic))
+;; (assert_return (invoke "f64.no_fold_div_one" (f64.const nan:0x4000000000000)) (f64.const nan:arithmetic))
 
 ;; Test that x/-1.0 is not folded to -x.