Skip to content

Conversation

@brendandahl
Copy link
Collaborator

@brendandahl brendandahl requested review from kripken and tlively August 22, 2024 18:38
;; 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

;; 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
@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