test/relaxed_dot: add i16-intermediate overflow boundary cases - #2228
test/relaxed_dot: add i16-intermediate overflow boundary cases#2228matthargett wants to merge 1 commit into
Conversation
The existing relaxed dot-product cases all use byte values whose i16 pair sums stay within i16 range, so an implementation that skips the i16 intermediate entirely (summing four byte products directly into i32 in the add variant) produces identical results on every existing input and the deviation is unobservable. Add a = b = -128 cases where each pair sum is 32768, one past the i16 maximum: for i16x8.relaxed_dot_i8x16_i7x16_s every allowed interpretation yields either -32768 (wrapping) or 32767 (saturating), and for i32x4.relaxed_dot_i8x16_i7x16_add_s the allowed lane values are -65536, -1, or 65534 -- while the no-i16-intermediate shape produces 65536, outside the set, so the boundary is now observable. Ported from WebAssembly/relaxed-simd#164 per maintainer guidance that the proposal repository is merged and inactive.
|
I did a quick review of V8 and most of the code should handle this correctly. I do see one potential issue with the wasm interpreter (not used by chrome) and |
rossberg
left a comment
rossberg
left a comment
There was a problem hiding this comment.
This looks okay, modulo the one case I comment on below.
The reference interpreter failed because the implementation of dot_add missed a saturation step. It's worrisome that this was not discovered before, but test coverage for relaxed SIMD is practically non-existent, as I have complained before. @dtig
I pushed a fix, so if you merge with main the new test should pass.
| ;; | ||
| ;; wrap+wrap = -65536 (PMADDUBSW + extadd, or s*s wrap, etc.) | ||
| ;; sat+sat = 65534 (s*s with saturating intermediate) | ||
| ;; wrap+sat = -1 |
There was a problem hiding this comment.
I don't see how this case is allowed. Both values have to be created consistently according to the spec, so a mixed wrap+sat addition ought to never arise.
3 participants
The existing relaxed dot-product cases all use byte values whose i16 pair sums stay within i16 range (e.g.
-128 * -127 * 2 = 32512). An implementation that skips the i16 intermediate — summing four byte products directly into i32 in the add variant — therefore produces identical results on every existing input, and the deviation from the spec'srelaxedi16 truncation point is unobservable. (I hit exactly this implementation bug while porting the instructions to an interpreter; the current suite passed it.)This adds
a = b = -128boundary cases where each pair sum is32768, one pastINT16_MAX:i16x8.relaxed_dot_i8x16_i7x16_s: every allowed interpretation yields-32768(wrapping, including the x86PMADDUBSWsigned-saturating path, which saturates at-32768for these inputs) or32767(saturating), asserted witheither.i32x4.relaxed_dot_i8x16_i7x16_add_swithc = 0: the pair-wise i16 results feedextadd_pairwise(which cannot overflow i32), so the allowed lane values are-65536(wrap+wrap),-1(wrap+sat), or65534(sat+sat). The no-i16-intermediate shape produces65536, which is outside the allowed set — making the truncation point observable.Both files validate with
wast2json --enable-all.I hit this when benchmarking SIMD workloads across multiple open source WASM runtimes on iPhone XS / Apple Watch SE2. Two of them had this implementation bug, but the CTS passed on both. (I already submitted fixes to both projects, and they're both merged.)
Originally opened as WebAssembly/relaxed-simd#164; relocated here per maintainer guidance that the proposal repository is merged and inactive.