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

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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

2 years ago
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...
(Assignee)

Comment 2

2 years ago
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
(Assignee)

Comment 3

2 years ago
Created attachment 8754384 [details] [diff] [review]
1.updatesh.importsh.patch
Attachment #8754384 - Flags: review?(arai.unmht)
(Assignee)

Comment 4

2 years ago
Created attachment 8754385 [details] [diff] [review]
2.meta.patch

Meta-patch: updates the patches to apply to fdlibm when doing an update.
Attachment #8754385 - Flags: review?(arai.unmht)
(Assignee)

Comment 5

2 years ago
Created attachment 8754386 [details] [diff] [review]
3.apply.patches.patch

Actually applies the meta-patches.
Attachment #8754386 - Flags: review?(arai.unmht)
(Assignee)

Comment 6

2 years ago
Created attachment 8754388 [details] [diff] [review]
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.
Attachment #8754388 - Flags: review?(sunfish)
(Assignee)

Comment 7

2 years ago
Created attachment 8754389 [details] [diff] [review]
5.copysign.patch

Copysign can be just implemented with bit twiddling.
Attachment #8754389 - Flags: review?(sunfish)
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.
Attachment #8754384 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8754385 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8754386 - Flags: review?(arai.unmht) → review+
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 ?
Attachment #8754388 - Flags: review?(sunfish) → review+
Comment on attachment 8754389 [details] [diff] [review]
5.copysign.patch

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

Looks good.
Attachment #8754389 - Flags: review?(sunfish) → review+
(Assignee)

Comment 12

2 years ago
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.
(Assignee)

Comment 13

2 years ago
(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.

Updated

2 years ago
Blocks: 1232205
You need to log in before you can comment on or make changes to this bug.