JIT: Improve codegen for Vector128/256.NarrowWithSaturation

[saucecontrol]

Resolves #116526

This adds some missing optimized paths for NarrowWithSaturation intrinsics in pre-AVX-512 environments.

Vector128.NarrowWithSaturation was fully accelerated for signed types but not unsigned:

static Vector128<ushort> NarrowSaturate(Vector128<uint> x, Vector128<uint> y)
	=> Vector128.NarrowWithSaturation(x, y);

        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 31

Vector256.NarrowWithSaturation was using the slow path for both signed and unsigned:

static Vector256<sbyte> NarrowSaturate(Vector256<short> x, Vector256<short> y)
	=> Vector256.NarrowWithSaturation(x, y);

-       vbroadcastss ymm0, dword ptr [reloc @RWD00]
-       vpmaxsw  ymm1, ymm0, ymmword ptr [rdx]
-       vbroadcastss ymm2, dword ptr [reloc @RWD04]
-       vpminsw  ymm1, ymm1, ymm2
-       vbroadcastss ymm3, dword ptr [reloc @RWD08]
-       vpand    ymm1, ymm1, ymm3
-       vpmaxsw  ymm0, ymm0, ymmword ptr [r8]
-       vpminsw  ymm0, ymm0, ymm2
-       vpand    ymm0, ymm0, ymm3
-       vpackuswb ymm0, ymm1, ymm0
+       vmovups  ymm0, ymmword ptr [rdx]
+       vpacksswb ymm0, ymm0, ymmword ptr [r8]
        vpermq   ymm0, ymm0, -40
        vmovups  ymmword ptr [rcx], ymm0
        mov      rax, rcx
        vzeroupper 
        ret      

-RWD00  	dd	FF80FF80h		;      -nan
-RWD04  	dd	007F007Fh		; 1.16633e-38
-RWD08  	dd	00FF00FFh		; 2.34184e-38
 
-; Total bytes of code 73
+; Total bytes of code 26

Full diffs

[Copilot]

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.NarrowWithSaturation codegen 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.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[saucecontrol]

cc @dotnet/jit-contrib this is ready for review.

Diffs

[JulieLeeMSFT]

@saucecontrol, please resolve comments.

[tannergooding]

The changes look correct to me, but I'm not really happy with NarrowWithSaturation going the opposite direction that we want 😅

I think the better long term thing here is to remove BaseTypeFromFirstArg, making it the default. We then only have BaseTypeFromSecondArg and can implicitly get it from the return type if no arguments exist. That helps force correctness, but also will involve a PR that finds all the intrinsics like Narrow that are incorrectly using the return type (and where it differs from the arg base type).

[saucecontrol]

That makes sense. For this PR, I was going for consistency, because NarrowWithSaturation falls back to Narrow, and the transition of base type between the two was confusing.

I'd be happy to take on switching all of the Narrow intrinsics for xarch and aarch to BaseTypeFromFirstArg in a follow-up since that's going to be bigger. Narrow is called from quite a few places (e.g. integral vector division) where adjustments will have to be made.

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary sign-off. Community PR improving some HWIntrinsic codegen

[kg]

nit: The way the individual simd sizes (16/32/64) are being handled here is a little magical and hard to follow, a brief comment explaining the approach might be nice.

[kg]

This in particular, I don't get. I assume it would make sense to me with a high level explanation comment. We're conditionally widening or narrowing based on simdSize? But this is NarrowWithSaturation, shouldn't we always be generating smaller or equal-sized vectors?

[saucecontrol]

I'll have to revisit this as part of the planned cleanup. I can add more comments then if that's ok with you. The basic idea is that we are narrowing two vectors into 1, so if the target isn't the max width supported by hardware, we can make the operation cheaper by building a single double-width vector and narrowing it with a single instruction. When that's not possible (because the sources are already the max supported vector width) we have to use two narrow instructions, which result in two half-size outputs that have to be rejoined. So we either need a temp size that's double or half the original size.

[dhartglassMSFT]

Sorry about the delay in reviewing - +1 to kg's comments here and the comment above. A brief explanation for this area of the change would be good. change LGTM otherwise

[kg]

Will leave the checkmark to Egor for now because I can't quite make sense of what's going on in this PR (will try again later though.) I didn't see any problems.

[tannergooding]

CC. @dhartglassMSFT for secondary review

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "9abaf4903d3c1be3acb127c0078d9e168b1c15c7",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "e00aafd1b832c884387c5c6ae2481137445123c9",
  "last_reviewed_commit": "9abaf4903d3c1be3acb127c0078d9e168b1c15c7",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "e00aafd1b832c884387c5c6ae2481137445123c9",
  "last_recorded_worker_run_id": "29682978344",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "9abaf4903d3c1be3acb127c0078d9e168b1c15c7",
      "review_id": 4730585950
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Vector128/256/512.NarrowWithSaturation had missing optimized paths in pre-AVX-512 environments. Unsigned narrowing on Vector128 and both signed/unsigned narrowing on Vector256 fell back to the slow clamp-based sequence instead of using the hardware PACKUSWB/PACKSSWB (and AVX2 permute) instructions, producing substantially larger code (e.g. 73 vs 26 bytes for the Vector256<sbyte> case). Resolves #116526.

Approach: The HW_Flag_BaseTypeFromFirstArg flag is removed from the NarrowWithSaturation list entries so simdBaseType now reflects the narrow return element type rather than the wide source type. A new getHWIntrinsicWidenType helper reconstructs the wide operand type where the intermediate operations still need it. The special-import logic is reorganized into: (1) a floating path, (2) a signed-small PackSignedSaturate path covering both 128- and 256-bit (with an AVX2_Permute4x64 lane fixup for 256-bit), (3) a consolidated AVX-512 ConvertTo*WithSaturation path that removes the previous 32/64 vs 16 code duplication, and (4) a fallback that clamps with min/max then packs (PackUnsignedSaturate for small unsigned, gtNewSimdNarrowNode otherwise). The now-unused isSupportedBaseType(NamedIntrinsic, CorInfoType) overload is deleted.

Summary: This is a clean, well-motivated refactor. The base-type semantics change (dropping HW_Flag_BaseTypeFromFirstArg) is applied consistently, and the new widen helper, assertions (assert(!varTypeIsSmall(simdBaseType) || varTypeIsUnsigned(simdBaseType)) in the fallback, and the domain asserts in getHWIntrinsicWidenType) correctly encode the invariants of each path. The removed CorInfoType overload of isSupportedBaseType has no remaining callers. The four narrowing paths partition the type space without gaps: signed small types use PackSignedSaturate, unsigned small types clamp + PackUnsignedSaturate, signed int clamps + NarrowNode, and all remaining cases use AVX-512 when available. Correctness is exercised by existing generic Vector128/256/512 narrowing tests, so the absence of new tests is acceptable for a codegen optimization of this kind. No blocking issues found. Recommend checking that CI jitdiff/antigen and the existing HWIntrinsic tests are green before merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 70.3 AIC · ⌖ 10.5 AIC · ⊞ 10K