1272640 - wasm: Implement {f32,f64}.{trunc,nearest,copysign}

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

These should be pretty easy to do:

- for trunc/nearest, we can use math functions, for a start. Then maybe optimize trunc/nearest/floor to use roundss/roundsd with a rounding mode, when it's available for x86. I haven't checked ARM yet.
- for copysign, we can reinterpret the inputs as uint32/uint64, then use a combination of and/or to extract the sign and replace it. Unless there is a corresponding instruction. That's what fdlibm does.

Comment 1

[Lars T Hansen [:lth]]

It would be lovely if these could be exposed in a platform-independent way in MacroAssembler.h from the outset - I'll need them too...

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Lars, my current plan is to use math.h functions for trunc/nearest. More precisely, fdlibm functions; so the same mechanism as for floor, etc. could be reused here. Although we could use fdlibm functions for copysign too, the inlining is quite easy: https://github.com/WebAssembly/spec/blob/master/ml-proto/spec/float.ml#L188

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.updatesh.importsh.patch

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.meta.patch

Meta-patch: updates the patches to apply to fdlibm when doing an update.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.apply.patches.patch

Actually applies the meta-patches.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.trunc.nearest.patch

This implements trunc/nearest in terms of VM calls. We can probably use roundss on recent intel chips, but I'd rather do that in a follow up.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5.copysign.patch

Copysign can be just implemented with bit twiddling.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3816ca4267f2

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8754384 [details] [diff] [review]
1.updatesh.importsh.patch

Review of attachment 8754384 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch :D

::: modules/fdlibm/import.sh
@@ +8,3 @@
>      REMOTE_FILENAME=$1
>      LOCAL_FILENAME=$2
> +    while true; do

shouldn't we abort script when curl fails several times?

::: modules/fdlibm/update.sh
@@ +11,5 @@
>      curl -s "${API_BASE_URL}/commits?path=lib/msun/src&per_page=1" \
>          | python -c 'import json, sys; print(json.loads(sys.stdin.read())[0]["sha"])'
>  }
>  
> +mv src/moz.build ./src_moz.build

thank you for following up :)

@@ +17,5 @@
>  BEFORE_COMMIT=$(get_commit)
>  sh ./import.sh
>  COMMIT=$(get_commit)
>  if [ ${BEFORE_COMMIT} != ${COMMIT} ]; then
>      echo "Latest commit is changed during import.  Please run again."

can you move the moz.build back here?
so that `mv src/moz.build ./src_moz.build` won't fail on next run.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8754388 [details] [diff] [review]
4.trunc.nearest.patch

Review of attachment 8754388 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. There's no test, though we will have good test coverage of these in the spec testsuite. With this patch and the copysign patch, is it enough to enable f32.wast and f64.wast in js/src/jit-test/tests/wasm/spec/list.js ?

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8754389 [details] [diff] [review]
5.copysign.patch

Review of attachment 8754389 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

And thank you for the review!

(In reply to Tooru Fujisawa [:arai] from comment #9)
> Comment on attachment 8754384 [details] [diff] [review]
> 1.updatesh.importsh.patch
> 
> Review of attachment 8754384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch :D
> 
> ::: modules/fdlibm/import.sh
> @@ +8,3 @@
> >      REMOTE_FILENAME=$1
> >      LOCAL_FILENAME=$2
> > +    while true; do
> 
> shouldn't we abort script when curl fails several times?

I've actually had a very flaky connection these days, and curl often resulted in CONNECTION_RESET failures, and that is why I have updated this code. However, curl failing might indicate many other things: e.g. the network can't be reached, or github being down for some reason. I guess the user can still force stop by ctrl+c in this case? Anyways, I don't have strong opinions here and would be okay reverting this chunk.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Dan Gohman [:sunfish] from comment #10)
> Comment on attachment 8754388 [details] [diff] [review]
> 4.trunc.nearest.patch
> 
> Review of attachment 8754388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. There's no test, though we will have good test coverage of these
> in the spec testsuite. With this patch and the copysign patch, is it enough
> to enable f32.wast and f64.wast in js/src/jit-test/tests/wasm/spec/list.js ?

Thank you for the reviews! Indeed all the testing is done in the spec tests, which are very cautious and tests many more things. Unfortunately, we can't enable the f32/f64 wast tests yet, because of custom NaNs.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/335c62fd3d98
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba34f7a79a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d6629e599e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f60ddfa778a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/740a2bf1fa9e

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/335c62fd3d98
https://hg.mozilla.org/mozilla-central/rev/7ba34f7a79a0
https://hg.mozilla.org/mozilla-central/rev/a7d6629e599e
https://hg.mozilla.org/mozilla-central/rev/f60ddfa778a9
https://hg.mozilla.org/mozilla-central/rev/740a2bf1fa9e