Repository navigation
[RemoveNonJSOps] Stub out trunc_sat and convert ops - #9313
Conversation
`wasm2js` and `I64ToI32Lowering` lower 32-bit and 64-bit `trunc_sat` instructions using non-saturating conversions (`~~x`, `~~x >>> 0`, and `lowerTruncFloatToInt`) that wrap rather than saturate on out-of-range inputs. Unlike trapping `trunc` instructions (which trap on out-of-range inputs in the interpreter and are not constant-folded by `Precompute`), `trunc_sat` instructions do not trap in WebAssembly and are folded to saturated constants when optimizing, causing differential mismatches in `Wasm2JS` fuzzing. Stub out all `trunc_sat` unary operations in `StubUnsupportedJSOpsPass::visitUnary`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the RemoveNonJSOps pass to stub out non-trapping float-to-int conversions (trunc_sat operations) and adds corresponding test coverage. The feedback suggests also stubbing out signed 64-bit integer to 32-bit float conversions (ConvertSInt64ToFloat32), as they suffer from the same double-rounding precision issues as the unsigned variant.
| case ConvertUInt64ToFloat32: | ||
| // See detailed comment in lowerConvertIntToFloat in | ||
| // I64ToI32Lowering.cpp. | ||
| case TruncSatSFloat32ToInt32: | ||
| case TruncSatUFloat32ToInt32: | ||
| case TruncSatSFloat64ToInt32: | ||
| case TruncSatUFloat64ToInt32: | ||
| case TruncSatSFloat32ToInt64: | ||
| case TruncSatUFloat32ToInt64: | ||
| case TruncSatSFloat64ToInt64: | ||
| case TruncSatUFloat64ToInt64: | ||
| // f32.convert_i64_u does not have perfect rounding in all cases (see | ||
| // lowerConvertIntToFloat in I64ToI32Lowering.cpp), and trunc_sat | ||
| // operations are lowered using non-saturating conversions in wasm2js | ||
| // and I64ToI32Lowering. |
There was a problem hiding this comment.
The signed 64-bit integer to 32-bit float conversion (ConvertSInt64ToFloat32 / f32.convert_i64_s) suffers from the exact same double-rounding precision issue as the unsigned version (ConvertUInt64ToFloat32 / f32.convert_i64_u). In I64ToI32Lowering.cpp, both are lowered by first converting to f64 and then demoting to f32, which can lead to rounding mismatches during fuzzing. We should stub out ConvertSInt64ToFloat32 as well. Please also add a corresponding test case in test/lit/passes/stub-unsupported-js.wast.
case ConvertSInt64ToFloat32:
case ConvertUInt64ToFloat32:
case TruncSatSFloat32ToInt32:
case TruncSatUFloat32ToInt32:
case TruncSatSFloat64ToInt32:
case TruncSatUFloat64ToInt32:
case TruncSatSFloat32ToInt64:
case TruncSatUFloat32ToInt64:
case TruncSatSFloat64ToInt64:
case TruncSatUFloat64ToInt64:
// f32.convert_i64_s and f32.convert_i64_u do not have perfect rounding
// in all cases (see lowerConvertIntToFloat in I64ToI32Lowering.cpp),
// and trunc_sat operations are lowered using non-saturating
// conversions in wasm2js and I64ToI32Lowering.There was a problem hiding this comment.
A surprisingly useful comment!
`f32.convert_i64_s` is lowered through an intermediate `f64` in `I64ToI32Lowering::lowerConvertIntToFloat` just like `f32.convert_i64_u`, so it suffers from the same double-rounding precision issue.
|
Should the title be |
tlively
changed the title
tlively
changed the title
wasm2jsandI64ToI32Loweringlower 32-bit and 64-bittrunc_satinstructions using non-saturating conversions (~~x,~~x >>> 0, andlowerTruncFloatToInt) that wrap rather than saturate on out-of-range inputs. Unlike trappingtruncinstructions (which trap on out-of-range inputs in the interpreter and are not constant-folded byPrecompute),trunc_satinstructions do not trap in WebAssembly and are folded to saturated constants when optimizing, causing differential mismatches inWasm2JSfuzzing. Stub out alltrunc_satunary operations inStubUnsupportedJSOpsPass::visitUnary.Also stub out
f32.convert_i64_sbecause its wasm2js implementation rounds twice and can therefore have slightly different results than the Wasm instruction.