[Wasm RyuJit] codegen for some intrinsics#124575
[Wasm RyuJit] codegen for some intrinsics#124575AndyAyersMS wants to merge 2 commits intodotnet:mainfrom
Conversation
Handle some math intrinsics that map to Wasm operations.
|
@dotnet/jit-contrib PTAL |
There was a problem hiding this comment.
Pull request overview
Adds initial WebAssembly (Wasm) RyuJIT support for lowering selected System.Math intrinsics directly to Wasm numeric instructions, enabling more operations to be emitted without helper calls.
Changes:
- Teach the importer which
System.Mathintrinsics are target-supported onTARGET_WASM. - Add Wasm codegen handling for
GT_INTRINSICand emit the corresponding Wasm instructions for supported intrinsics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/coreclr/jit/importercalls.cpp |
Adds a TARGET_WASM arm to Compiler::IsTargetIntrinsic for a set of System.Math intrinsics. |
src/coreclr/jit/codegenwasm.cpp |
Handles GT_INTRINSIC in Wasm codegen and introduces CodeGen::genIntrinsic to emit Wasm ops for supported intrinsics. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I think you'd want @tannergooding's review. These intrinsics are tricky around IEEE754. E.g. it seems f32.max in WASM corresponds to IEEE 754-2019's maximum. Is it the same for Math.Max? Same concerns for min, round |
| case PackIntrinsicAndType(NI_System_Math_Max, TYP_FLOAT): | ||
| ins = INS_f32_max; | ||
| break; | ||
| case PackIntrinsicAndType(NI_System_Math_Max, TYP_DOUBLE): | ||
| ins = INS_f64_max; | ||
| break; | ||
|
|
||
| case PackIntrinsicAndType(NI_System_Math_Min, TYP_FLOAT): | ||
| ins = INS_f32_min; | ||
| break; | ||
| case PackIntrinsicAndType(NI_System_Math_Min, TYP_DOUBLE): | ||
| ins = INS_f64_min; | ||
| break; |
There was a problem hiding this comment.
Math.{F}.Min/Max are specified/implemented to return the NaN operand verbatim (preserving payload and, when both are NaN, preserving the first operand). Wasm f32.min/f32.max and f64.min/f64.max allow NaN canonicalization and generally won't preserve NaN payloads, so directly emitting these ops can change observable results. Consider either (a) not treating NI_System_Math_Min/Max as a target intrinsic on Wasm yet, or (b) lowering to a sequence that explicitly checks for NaN and selects the original NaN input to preserve payload semantics before/after using min/max.
| genConsumeOperands(treeNode); | ||
|
|
||
| // Handle intrinsics that can be implemented by target-specific instructions | ||
| instruction ins; |
There was a problem hiding this comment.
instruction ins; is left uninitialized and then assigned in the switch. Some toolchains warn on this pattern even with unreached() in the default case. Initializing ins to INS_invalid (or similar) avoids potential build breaks from -Wmaybe-uninitialized and matches the pattern used in other backends.
| instruction ins; | |
| instruction ins = INS_invalid; |
| // TODO-WASM-CQ: we can likely support more intrinsics here | ||
| switch (intrinsicName) | ||
| { | ||
| case NI_System_Math_Abs: | ||
| case NI_System_Math_Ceiling: | ||
| case NI_System_Math_Floor: | ||
| case NI_System_Math_Max: | ||
| case NI_System_Math_Min: | ||
| case NI_System_Math_Round: | ||
| case NI_System_Math_Sqrt: | ||
| case NI_System_Math_Truncate: | ||
| return true; |
There was a problem hiding this comment.
IsTargetIntrinsic now claims NI_System_Math_Min/Max are implemented by target instructions on Wasm. If codegen uses Wasm min/max, be aware that Wasm min/max do not preserve NaN payloads while System.Math/MathF implementations intentionally return a NaN argument verbatim; this can be an observable behavior difference. Consider dropping Min/Max from the Wasm target-intrinsic list until codegen preserves NaN payload semantics (or implement the required NaN handling sequence in codegen).
|
|
||
| #elif defined(TARGET_WASM) | ||
|
|
||
| // TODO-WASM-CQ: we can likely support more intrinsics here |
There was a problem hiding this comment.
For example:
case NI_System_Math_CopySign: // f32.copysign / f64.copysign
case NI_System_Math_MaxNative: // f32.max / f64.max
case NI_System_Math_MinNative: // f32.min / f64.mincase PackIntrinsicAndType(NI_System_Math_CopySign, TYP_FLOAT):
ins = INS_f32_copysign;
break;
case PackIntrinsicAndType(NI_System_Math_CopySign, TYP_DOUBLE):
ins = INS_f64_copysign;
break;|
The Wasm spec says:
If we must explicitly check for and propagate NaN inputs this may tip the scale in favor of always using a helper call (especially for binops). |
|
Notably, the NaN propagation issue affects all FP operations, not just intrinsics. What are our actual semantic requirements for it (required for all ops, required only for min/max, required for all |
Handle some math intrinsics that map to Wasm operations.