Fuzzer: Add SIMD support to DeNaN #6318
Conversation
src/passes/DeNaN.cpp
Outdated
| uint8_t zero128[16] = {}; | ||
| // InvalidBinary is passed here as the condition in this case is more | ||
| // complex and computed above. | ||
| add(deNan128, Type::v128, Literal(zero128), InvalidBinary); |
There was a problem hiding this comment.
How about making the op argument a std::optional? That would take less explaining than passing InvalidBinary here, I think.
src/passes/DeNaN.cpp
Outdated
| } | ||
|
|
||
| // Check if a contant v128 may contain f32 or f64 NaNs. | ||
| bool maybeNaN(Const* c) { |
There was a problem hiding this comment.
Maybe call this hasNaNLane or hasNaN? I don't think maybe is quite accurate.
|
|
||
| ;; This is not a NaN. (We do still emit a call for it atm, FIXME) | ||
| (drop | ||
| (v128.const i32x4 0x00000001 0x00000002 0x00000003 0x00000004) |
There was a problem hiding this comment.
It would be useful to write the tests using f32x4 and f64x2 shapes rather than i32x4 so we can better see what values are being passed. You can use the --new-wat-parser and its extensive support for the standard nan formats if you want.
There was a problem hiding this comment.
I was actually thinking it is nice to see the actual bits because of how they are used in the pass (the explanation about the f32 and f64 bits overlapping etc.)?
There was a problem hiding this comment.
To be more specific, in this test code:
;; This is an f64 NaN and also an f32. It will become 0's.
(drop
(v128.const i32x4 0xffffffff 0x00000002 0x00000003 0x00000004)
)
;; This is an f32 NaN and not an f64. It will also become 0's.
(drop
(v128.const i32x4 0x00000001 0xffffffff 0x00000003 0x00000004)
)It is nice to see the bits, because that allows the comments to clarify how the same bits can be both f32 and f64 nans, or not.
There was a problem hiding this comment.
I would be more convinced by this if readers were likely to know off the top of their heads what bit patterns correspond to NaNs. I know I don't :)
There was a problem hiding this comment.
Heh, I don't know that either, but I mentioned it in the pass (8 vs 11 top non-sign bits).
| (func $foo128 (param $x v128) (result v128) | ||
| ;; The incoming param will be de-naned. | ||
|
|
||
| ;; This is not a NaN. (We do still emit a call for it atm, FIXME) |
There was a problem hiding this comment.
No good reason - that's what the code did for f32 and f64 before this PR. I can fix it later.
|
Adding some changes from feedback, and responses to other feedback. |
It already sanitized away NaNs in f32 and f64, but was missing SIMD. This matters
when comparing results between VMs due to nondeterminism, which is tested
more thanks to #6312