Wasmtime GitHub notifications bot (Nov 29 2020 at 08:32): 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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 01 2020 at 18:59):jlb6740 has marked PR #2457 as ready for review.
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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 02 2020 at 07:04):jlb6740 requested cfallin for a review on PR #2457.
Wasmtime GitHub notifications bot (Dec 02 2020 at 07:04):jlb6740 requested cfallin and abrown for a review on PR #2457.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):abrown submitted PR Review.
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):abrown created PR Review Comment:
This comment is still helpful?
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:19):abrown created PR Review Comment:
I noticed that
RoundImmdoesn'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.
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
ROUNDSDandROUNDSS--@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...
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:30):jlb6740 submitted PR Review.
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:31):jlb6740 submitted PR Review.
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:32):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:32):jlb6740 created PR Review Comment:
:+1:
Wasmtime GitHub notifications bot (Dec 02 2020 at 18:12):jlb6740 submitted PR Review.
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 18:56):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 18:56):jlb6740 created PR Review Comment:
:+1: Will add a note.
Wasmtime GitHub notifications bot (Dec 02 2020 at 19:40):bnjbvr submitted PR Review.
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.
Wasmtime GitHub notifications bot (Dec 02 2020 at 19:43):jlb6740 submitted PR Review.
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.
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.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 02 2020 at 21:44):jlb6740 merged PR #2457.
Last updated: Nov 22 2024 at 17:03 UTC