From 524bbb48aa92c346204c15fd1a04c9bb3b35554f Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 14 Jul 2020 09:35:22 -0700 Subject: [PATCH 1/4] Make float divide by 1 not change nan bits --- src/wasm/literal.cpp | 14 ++++++++++++++ test/passes/fuzz-exec_O.txt | 19 +++++++++++++++++++ test/passes/fuzz-exec_O.wast | 15 +++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/src/wasm/literal.cpp b/src/wasm/literal.cpp index 29d0477232e..d055be818e4 100644 --- a/src/wasm/literal.cpp +++ b/src/wasm/literal.cpp @@ -868,6 +868,16 @@ 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. + if (rhs == 1) { + return Literal(lhs); + } return Literal(lhs / rhs); default: WASM_UNREACHABLE("invalid fp classification"); @@ -896,6 +906,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 841507f6002..9c82805860c 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 3d03de71479..44bbd71016c 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) + ) +) From 51ec8e3985ed511215c412f4de9f17d561cc3913 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 14 Jul 2020 09:36:22 -0700 Subject: [PATCH 2/4] comment --- src/wasm/literal.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/wasm/literal.cpp b/src/wasm/literal.cpp index d055be818e4..f0a065c45cd 100644 --- a/src/wasm/literal.cpp +++ b/src/wasm/literal.cpp @@ -874,7 +874,9 @@ Literal Literal::div(const Literal& other) const { // 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. + // 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); } From 96050e8c774fa51ab5197aea98520e6e6909f521 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 14 Jul 2020 10:27:28 -0700 Subject: [PATCH 3/4] disable old spec test --- test/spec/old_float_exprs.wast | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/spec/old_float_exprs.wast b/test/spec/old_float_exprs.wast index 7900832b0a8..f12ee82c8bd 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 thi 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. From 505e6012801e8817d58bd47a5673aa0610f49d9a Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 15 Jul 2020 10:48:40 -0700 Subject: [PATCH 4/4] Update test/spec/old_float_exprs.wast Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com> --- test/spec/old_float_exprs.wast | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec/old_float_exprs.wast b/test/spec/old_float_exprs.wast index f12ee82c8bd..44515a32e80 100644 --- a/test/spec/old_float_exprs.wast +++ b/test/spec/old_float_exprs.wast @@ -133,7 +133,7 @@ (f64.div (local.get $x) (f64.const 1.0))) ) -;; XXX BINARYEN: disable thi test, as we have testing for the more strict property +;; 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))