-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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] Add end to end tests of all the F16x8 intrinisics. #22530
Conversation
4b8db3a
to
78e4b03
Compare
A few todos: - test at other optimization levels - enable liftoff when FP16 is fixed in V8 - test missing instructions
78e4b03
to
b2e2f11
Compare
test/test_fp16.c
Outdated
a = wasm_f16x8_nearest(wasm_f16x8_splat(1.5f)); | ||
assert_all_lanes_eq(a, 2.0f); | ||
|
||
a = wasm_f16x8_eq(wasm_f16x8_splat(2.0f), wasm_f16x8_splat(3.0f)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth covering all the interesting cases for the comparisons to ensure we are getting the right comparison at runtime.
}) | ||
def test_fp16(self, opts): | ||
self.v8_args += ['--experimental-wasm-fp16'] | ||
# TODO Remove this. Liftoff is currently broken for this test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, do you know what specifically is broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/test_other.py
Outdated
@requires_v8 | ||
@parameterized({ | ||
'': [[]], | ||
'O2': [['-O2']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe -O3
for the most coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you recommend in addition to or in replace of O2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just replace it, as it's practically a superset of -O2
.
A few todos: