-
Notifications
You must be signed in to change notification settings - Fork 43
Extend assert_return_arithmetic_nan
and assert_return_canonical_nan
for SIMD types
#137
Comments
I maintain that a type suffix on the assertion name itself makes most sense. From @rossberg in WAVM/WAVM#187 (comment):
Other Also I find that having the lane interpretation in the assertion name is more readable, but that doesn't seem worth arguing about since no one is going to change their mind over aesthetics. |
If I follow that correctly, then the other side of this argument is to move all of the distinguishing elements into the parameter list, like |
@abrown I like that idea, but I wouldn't want to apply it to only SIMD asserts when we have a different convention for scalar asserts. |
Ah, yeah; and perhaps it makes sense to keep it separate from |
That would seem to fit well with |
Interesting! I like the idea. Let's go further even, since the growing number of special variants of
where |
SGTM. Would it be a problem to perform this refactoring across the existing test suite? |
Don't think so. The main bulk of the work would be the nan returns in the main spec repo. But they're concentrated in a few files and can be handled by a regexp. Tools need to be adapted, though. How many tools are there now that consume .wast files? |
I know Binaryen and WABT both have interpreters for running spec tests. My impression is that most engines also run spec tests. But updating the spec repo will only cause major headaches for projects that automatically pull in the tip-of-tree test suite, and I know that neither Binaryen nor WABT do that, so this change should be manageable at least for those two projects. I don't know about engines. |
Thinking about this further, I think even my suggestion above is still not exactly right. For SIMD, you actually want to be able to specify NaNs per lane, e.g., to express the result of a div instruction that only has NaN in some of the lanes. So the syntax for results should be a kind of "pattern" for values:
Here, With this you could e.g. write
|
I like this scheme, but if there is an appetite for something simpler, extract_lane instructions could still be used for asserts with heterogeneous lanes. |
@tlively, fair enough, though of course, that would be arguing against extending assert_return_*_nan to SIMD in the first place. ;) |
Not necessarily. Back on the original thread about this we had decided to use new SIMD asserts for homogeneous vectors and extract_lanes for heterogeneous vectors, because it wasn’t worth complicating the asserts to handle heterogeneous vectors themselves. |
I think this is closed by WebAssembly/spec#1104. |
WAST has directives to check that NaNs returned by tests have the correct bits:
assert_return_arithmetic_nan
andassert_return_canonical_nan
. With the inclusion of SIMD instructions, these directives need to understand what type of vector is returned by a test to correctly check whether NaNs in the vector lanes are correct (e.g. anf32x4
filled with NaNs is different than anf64x2
filled with NaNs). There are two proposals out there for extending the directive:(assert_return_arithmetic_nan_f32x4 (invoke ...))
(assert_return_canonical_nan (invoke ...) f64x2)
I am raising the issue here to solicit comments before the SIMD spec tests are merged that require this feature. @Honry has worked on this in WAVM/WAVM#187 (see discusison by @AndrewScheidecker and @rossberg) and I have a PR ready to merge in bytecodealliance/wat#24. Please tag whoever else might have an opinion on the issue.
The text was updated successfully, but these errors were encountered: