Stream: git-wasmtime

Topic: wasmtime / PR #2457 Adds x86 SIMD support for Ceil, Floor...


view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2020 at 08:32):

jlb6740 opened PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2020 at 23:17):

jlb6740 updated PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2020 at 23:42):

jlb6740 updated PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2020 at 07:37):

jlb6740 updated PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2020 at 18:56):

jlb6740 updated PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2020 at 18:59):

jlb6740 has marked PR #2457 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 04:58):

jlb6740 updated PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 07:04):

jlb6740 requested cfallin for a review on PR #2457.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 07:04):

jlb6740 requested cfallin and abrown for a review on PR #2457.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown created PR Review Comment:

I think these two tests should be merged with the block above which already has the "check for x64 or aarch64" logic.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown created PR Review Comment:

This comment is still helpful?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown created PR Review Comment:

I think you can use the shorter RegMem::from(dst) here.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown created PR Review Comment:

I noticed that RoundImm doesn't mention bit 2 (indicating where rounding is controlled) and bit 3 (the precision mask). Do you want to document this even if we don't implement that part? Right now we are implicitly setting both of those bits to 0.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown created PR Review Comment:

I know this isn't part of the PR, but it is curious that these are implemented as lib calls instead of using ROUNDSD and ROUNDSS--@bnjbvr, do you know why this is? I wonder if there is some NaN handling there that we should also be doing for the packed version...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):

abrown created PR Review Comment:

For reference, here is the NaN paragraph from the manual for ROUNDPD:

The Precision Floating-Point Exception is signaled according to the immediate operand. If any source operand is an SNaN then it will be converted to a QNaN. If DAZ is set to ‘1 then denormals will be converted to zero before rounding.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:30):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:30):

jlb6740 created PR Review Comment:

:+1: Yeah, in cases where there is the same gate that makes sense.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:31):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:31):

jlb6740 created PR Review Comment:

:+1: Yes, will remove. I had it removed in an earlier iteration but in that patch I'd also removed the gate. When I reverted it all I failed to remove this again.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:32):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 17:32):

jlb6740 created PR Review Comment:

:+1:

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 18:12):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 18:12):

jlb6740 created PR Review Comment:

Using ROUNDSD and ROUNDSS is mentioned as a TODO as if it is just missing. I thought these were implemented this way because SSE2 was the minimum supported and because it was faster to implement. I was trying to discern/confirm what the spec wanted beforehand (even in the non-scalar before Ben implemented) and I didn't read in any compatibility with the spec and the line about conversion to QNaN but perhaps Ben has a better idea.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 18:56):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 18:56):

jlb6740 created PR Review Comment:

:+1: Will add a note.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 19:40):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 19:40):

bnjbvr created PR Review Comment:

Yes, roundss/roundsd are sse4.1 or si, if I remember correctly? Definitely worth adding for the scalar case when the right SSE level is available!

I recall that the wasm spec follows what the hardware does for NaNs for these opcodes, at least in the scalar case.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 19:43):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 19:43):

jlb6740 created PR Review Comment:

:+1: Yes, should be a straight forward follow-up patch. I can submit that.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 20:50):

jlb6740 updated PR #2457 from ceil-floor-trunc-nearest to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2020 at 21:44):

jlb6740 merged PR #2457.


Last updated: Dec 23 2024 at 12:05 UTC