Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[interpreter] f32.convert_u/i64 and f32_convert_s/i64 have incorrect rounding #421

Closed
sunfishcode opened this issue Feb 9, 2017 · 7 comments · Fixed by #1021
Closed
Milestone

Comments

@sunfishcode
Copy link
Member

sunfishcode commented Feb 9, 2017

The implementation of f32.convert_u/i64 has incorrect rounding in some cases. For example, these tests should pass but currently fail:

(assert_return (invoke "f32.convert_u_i64" (i64.const 0x7fffff4000000001)) (f32.const 0x1.fffffep+62))
(assert_return (invoke "f32.convert_u_i64" (i64.const 0x8000008000000001)) (f32.const 0x1.000002p+63))

A fix analogous to #420 is not sufficient, because OCaml lacks 32-bit float types, so it would round to 64-bit float first, then round to 32-bit float in a separate step, leading to incorrect rounding. For example, the above tests still fail.

@sunfishcode
Copy link
Member Author

Here's another interesting boundary case:

(assert_return (invoke "f32.convert_u_i64" (i64.const 0xfffffe8000000001)) (f32.const 0x1.fffffep+63))

@bnjbvr
Copy link
Member

bnjbvr commented Feb 24, 2017

Another signed one:

(assert_return (invoke "f32.convert_s_i64" (i64.const 0x358a09a000000002)) (f32.const 3857906751034621952))

@sunfishcode sunfishcode changed the title f32.convert_u/i64 has incorrect rounding f32.convert_u/i64 and f32_convert_s/i64 have incorrect rounding Feb 24, 2017
@sunfishcode
Copy link
Member Author

Ah, the signed implementation is also using double rounding.

Here's a more comprehensive collection of boundary condition testcases. For both unsigned and signed, for the minimum value, the maximum value in the low half of the domain, the minimum value in the high half of the domain, and the maximum value, where double rounding is observable:

(assert_return (invoke "f32.convert_u_i64" (i64.const 0x0020000020000001)) (f32.const 0x1.000002p+53))
(assert_return (invoke "f32.convert_u_i64" (i64.const 0x7fffffbfffffffff)) (f32.const 0x1.fffffep+62))
(assert_return (invoke "f32.convert_u_i64" (i64.const 0x8000008000000001)) (f32.const 0x1.000002p+63))
(assert_return (invoke "f32.convert_u_i64" (i64.const 0xfffffe8000000001)) (f32.const 0x1.fffffep+63))
(assert_return (invoke "f32.convert_s_i64" (i64.const 0x8000004000000001)) (f32.const -0x1.fffffep+62))
(assert_return (invoke "f32.convert_s_i64" (i64.const 0xffdfffffdfffffff)) (f32.const -0x1.000002p+53))
(assert_return (invoke "f32.convert_s_i64" (i64.const 0x0020000020000001)) (f32.const 0x1.000002p+53))
(assert_return (invoke "f32.convert_s_i64" (i64.const 0x7fffff4000000001)) (f32.const 0x1.fffffep+62))

@rossberg rossberg changed the title f32.convert_u/i64 and f32_convert_s/i64 have incorrect rounding [interpreter] f32.convert_u/i64 and f32_convert_s/i64 have incorrect rounding May 18, 2017
@ericprud
Copy link

@sunfishcode, @bnjbvr, has this been addressed? Can this issue be closed?

@rossberg
Copy link
Member

AFAIK, it hasn't been addressed, since there is no easy way to fix it in Ocaml atm, short of implementing f32 ourselves. Let's leave it open.

@sunfishcode
Copy link
Member Author

I agree. Here's a standalone wast testcase of the tests listed above, updated for the new operator names, which fails in the current spec interpreter but which should pass, and does pass in Wasmtime, the wabt interpreter, and probably other implementations.

(module
  (func (export "f32.convert_i32_s") (param $x i32) (result f32) (f32.convert_i32_s (local.get $x)))
  (func (export "f32.convert_i64_s") (param $x i64) (result f32) (f32.convert_i64_s (local.get $x)))
  (func (export "f64.convert_i32_s") (param $x i32) (result f64) (f64.convert_i32_s (local.get $x)))
  (func (export "f64.convert_i64_s") (param $x i64) (result f64) (f64.convert_i64_s (local.get $x)))
  (func (export "f32.convert_i32_u") (param $x i32) (result f32) (f32.convert_i32_u (local.get $x)))
  (func (export "f32.convert_i64_u") (param $x i64) (result f32) (f32.convert_i64_u (local.get $x)))
  (func (export "f64.convert_i32_u") (param $x i32) (result f64) (f64.convert_i32_u (local.get $x)))
  (func (export "f64.convert_i64_u") (param $x i64) (result f64) (f64.convert_i64_u (local.get $x)))
)

(assert_return (invoke "f32.convert_i64_u" (i64.const 0x0020000020000001)) (f32.const 0x1.000002p+53))
(assert_return (invoke "f32.convert_i64_u" (i64.const 0x7fffffbfffffffff)) (f32.const 0x1.fffffep+62))
(assert_return (invoke "f32.convert_i64_u" (i64.const 0x8000008000000001)) (f32.const 0x1.000002p+63))
(assert_return (invoke "f32.convert_i64_u" (i64.const 0xfffffe8000000001)) (f32.const 0x1.fffffep+63))
(assert_return (invoke "f32.convert_i64_s" (i64.const 0x8000004000000001)) (f32.const -0x1.fffffep+62))
(assert_return (invoke "f32.convert_i64_s" (i64.const 0xffdfffffdfffffff)) (f32.const -0x1.000002p+53))
(assert_return (invoke "f32.convert_i64_s" (i64.const 0x0020000020000001)) (f32.const 0x1.000002p+53))
(assert_return (invoke "f32.convert_i64_s" (i64.const 0x7fffff4000000001)) (f32.const 0x1.fffffep+62))

The spec interpreter prints:

Result: 9.007_199_254_74e+15 : f32
Expect: 9.007_200_328_48e+15 : f32
test.wast:12.1-12.103: assertion failure: wrong return values

@rossberg
Copy link
Member

I think I have a simple fix for this, see #1021. @sunfishcode, PTAL.

dhil pushed a commit to dhil/webassembly-spec that referenced this issue Oct 3, 2023
[test] Add tests for packed struct fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants