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

[FP16] Implement unary operations. #6867

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

brendandahl
Copy link
Collaborator

@@ -147,3 +154,36 @@
(v128.const i16x8 0x5140 0xfe00 0x7c00 0x3e00 0 0x3c00 0 0x3c00))
;; nan -nan inf 1.5 0 1 1 1
(v128.const i16x8 0x7e00 0xfe00 0x7c00 0x3e00 0 0x3c00 0x3c00 0x3c00))

;; unary arithmetic
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these tests result in -nan. Should I be avoiding those since they're non-deterministic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We support using nan:canonical and nan:arithmetic and nan:<payload> constants in spec tests, so perhaps that is an option as well. Although that might not support f16 constants out of the box, and either the interpreter or the wast runner seems to have some bugs in this area, so that may also be a problem.

If it's a pain, I'd say don't worry about changing it for now.

Comment on lines +1918 to +1920
Literal Literal::sqrtF16x8() const {
return unary<8, &Literal::getLanesF16x8, &Literal::sqrt, &toFP16>(*this);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this convert the f16s to f32s and then do the sqrt? IIUC, that would round incorrectly in some cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the paper in this area did prove that f32 sqrt can be done in f64 safely, and I'd guess the same should be true of f16 to f32.

But in this case there are only 2^16 f16 values, so we can just check them all 😎

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my quick test using soft float, the results appear the same for sqrt:
https://gist.github.com/brendandahl/10b3ab8d4ac8abc7212e862833322c26

@@ -147,3 +154,36 @@
(v128.const i16x8 0x5140 0xfe00 0x7c00 0x3e00 0 0x3c00 0 0x3c00))
;; nan -nan inf 1.5 0 1 1 1
(v128.const i16x8 0x7e00 0xfe00 0x7c00 0x3e00 0 0x3c00 0x3c00 0x3c00))

;; unary arithmetic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We support using nan:canonical and nan:arithmetic and nan:<payload> constants in spec tests, so perhaps that is an option as well. Although that might not support f16 constants out of the box, and either the interpreter or the wast runner seems to have some bugs in this area, so that may also be a problem.

If it's a pain, I'd say don't worry about changing it for now.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % open discussions

@brendandahl brendandahl merged commit 6c2d0e2 into WebAssembly:main Aug 27, 2024
13 checks passed
@gkdn gkdn mentioned this pull request Aug 31, 2024
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 this pull request may close these issues.

3 participants