JIT: Improve codegen for Vector128/256.NarrowWithSaturation#126226
JIT: Improve codegen for Vector128/256.NarrowWithSaturation#126226saucecontrol wants to merge 6 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR refactors x86/x64 SIMD vector conversion intrinsic selection into a shared helper and adds missing fast paths for Vector128/256.NarrowWithSaturation in non-AVX512 environments, reducing instruction count and code size for several narrow-with-saturation cases.
Changes:
- Introduce
GenTreeHWIntrinsic::GetHWIntrinsicIdForVectorConvert(...)to centralize lookup of conversion-related intrinsics (including optional saturating preference). - Improve
Vector128/256.NarrowWithSaturationcodegen on pre-AVX512 machines by using pack-based sequences where applicable. - Refactor existing conversion/widen/narrow construction to use the shared lookup helper instead of duplicated switch logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/coreclr/jit/hwintrinsicxarch.cpp | Uses the new conversion lookup helper and adds optimized pack-based paths for NarrowWithSaturation on non-AVX512. |
| src/coreclr/jit/gentree.h | Declares the new shared vector-convert intrinsic lookup helper. |
| src/coreclr/jit/gentree.cpp | Implements the helper and refactors several SIMD convert/narrow/widen paths to use it. |
|
cc @dotnet/jit-contrib this is ready for review. |
| var_types sourceType, | ||
| var_types targetType, | ||
| unsigned simdSize, | ||
| bool preferSaturating = false, |
There was a problem hiding this comment.
nit: I don't think prefer is a good name and its likely to cause confusions/bugs in the future.
We have two types of narrowing, truncation and saturation, where each is explicitly required for one of the publicly exposed APIs and so we need a way to emit each deterministically.
We then have an optimization where we want whatever is fastest under the presumption that everything is in range, which is functionally a NarrowEstimate API (going based on the naming we use elsewhere) and which is where the knowledge of "what is fastest" can be handled (i.e. just use the truncating APIs on AVX-512 hardware and the saturating ones downlevel).
I think we want to split this into those three, accordingly, and if anything this helper should simply tell you whether it gave back a saturating or truncating instruction; there shouldn't be any "prefer" mechanic because such a preference is going to be driven based on some need or optimization instead.
There was a problem hiding this comment.
So... where I was going with this is that in quite a few places, we would prefer to have an instruction that saturates, but if there's an instruction that does the conversion but doesn't saturate, we can still use that and just the do saturation before or after. Or we may choose not to saturate. i.e. it really is a preference, with a choice to be made after we see what the lookup found.
What I was originally looking at was the floating->integral vector conversions, where we have e.g. ConvertToInt64Native and ConvertToInt64, with the latter guaranteeing a saturating conversion and the former making that undefined. The behaviors of those are not necessarily expected to be the same, but they're more inconsistent today than they need to be.
- On Arm64,
ConvertToInt64andConvertToInt64Nativedo the same thing, because the native instructions always saturate. - On x86/64 with AVX-512 plus, we saturate
ConvertToInt64but notConvertToInt64Native(even though saturation is free on AVX10v2). - On x86/64 without AVX-512, we saturate both
ConvertToInt64andConvertToInt64Native, because we fall back to a scalar cast loop for both, and the scalar cast saturates. But even this is inconsistent, because we don't saturateConvertToInt32Nativesince that has a legacy native instruction and is handled as intrinsic.
What I envisioned was that we would 'prefer' a saturating conversion for both, which would change the behavior of Avx10v2 to match what we do on Arm64, because honestly, the saturation is just the most sensible behavior. I would then add emulation for the pre-AVX-512 paths (which, again, today fall back to unrolled scalar implementations), which would also saturate, because it's really no more expensive to saturate than it is to emulate exactly what the native instructions do. That emulation would be used for both ConvertToInt64 and ConvertToInt64Native.
This pattern also fits for NarrowSaturating, where we would 'prefer' a single instruction that does it directly, but would accept a non-saturating instruction since we can emulate that before narrowing.
Anyway, if you don't like that pattern, I'm fine to change it. I can still do what I wanted to do -- it would just be two instruction lookups in those places instead of one with a flag.
There was a problem hiding this comment.
I should also say that under the model I envisioned, gtNewSimdCvtNode and gtNewSimdCvtNativeNode would roll up into the same thing, where the Native implementation is expressed only as a preference for a saturating instruction but would accept non-saturating if that's all we have available.
There was a problem hiding this comment.
I think the issue is that's a fairly centric view to the floating-point conversions. In the inverse direction you have the normal integer to integer narrow operations where users typically expect truncation instead, outside of specific scenarios. It's also where Arm64 provides the truncating versions out of the box and where x64 provides faster truncating versions on modern hardware, so in that case you don't want to prefer saturation.
So I think it really comes down to "you require a behavior" or "you don't" and in the latter its typically you want whatever is fastest (and typically that's because you have some known invariant, which the JIT can't know, that its in range and so edges aren't relevant).
So I think the simplest thing to do here is to do what we do elsewhere, which is define Narrow (truncating), NarrowSaturating, and NarrowEstimate helpers. The first two do exactly what they're described to do, whatever way makes it correct. The latter then dispatches to one of the two based on the types provided and knowing whichever is faster for the underlying hardware (i.e. it may truncate, it may saturate, it may even change based on what ISAs are enabled/disabled). -- and in the case both are equally fast, then it prefers whatever is consistent with the non-estimate behavior (so truncation for integer->integer and saturation for float->integer, with integer->float only having one correct behavior which is saturating)
There was a problem hiding this comment.
Ok, I like the way that sounds. Will see what it looks like in code.
Resolves #116526
This adds some missing optimized paths for
NarrowWithSaturationintrinsics in pre-AVX-512 environments.Vector128.NarrowWithSaturationwas fully accelerated for signed types but not unsigned:vbroadcastss xmm0, dword ptr [reloc @RWD00] vpminuw xmm1, xmm0, xmmword ptr [rdx] - vpand xmm1, xmm1, xmm0 - vpminuw xmm2, xmm0, xmmword ptr [r8] - vpand xmm0, xmm2, xmm0 + vpminuw xmm0, xmm0, xmmword ptr [r8] vpackuswb xmm0, xmm1, xmm0 vmovups xmmword ptr [rcx], xmm0 mov rax, rcx ret RWD00 dd 00FF00FFh ; 2.34184e-38 -; Total bytes of code 39 +; Total bytes of code 31Vector256.NarrowWithSaturationwas using the slow path for both signed and unsigned:The bulk of the code changes here are from a refactoring of the intrinsic lookup for various vector convert ops. I'm filling in some of the optimization gaps in these intrinsics, and the tangle of logic required to select the right intrinsic is spread out in various places. Having it in a shared method will make it easier to complete the other changes I have planned.
The refactor is split into the first commit, which is zero-diff. Second commit on include the codegen improvements.
Full diffs