Use NarrowNative for narrowing operations

[xtqqczze]

This PR introduces a new helper method, NarrowNative, to handle narrowing operations more consistently. It replaces prior logic (previously using ExtractAsciiVector) with this unified implementation.

Once #118583 is completed, the conditional compilation blocks can be removed, using the unified implementation for all targets.

https://csharp.godbolt.org/z/dc1zn9jMb

[EgorBo]

Wonder if that extra AND is a big deal..

Perhaps, it should be called NarrowUnsafe ? and moved to Vector128 as an internal helper. cc @tannergooding

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-encoding
See info in area-owners.md if you want to be subscribed.

[tannergooding]

Wonder if that extra AND is a big deal..

👍

I don't expect this to have any significant impact and we'd overall prefer less private/internal helpers where possible.

I think we should just keep it as is and live with the single extra and instruction; likely moving other APIs to do the same over time.

Perhaps, it should be called NarrowUnsafe

I don't think this is one that would be good for an unsafe/estimate API as we typically only use such APIs for handling things that live as undefined behavior. Narrowing basically comes down to only having well-defined behavior (truncation or saturation). I don't think we'd necessarily want one that does one or the other based on whatever is "fastest".

For many of the cases using ExtractToAsciiVector, we could even extend the existing range support to allow implicit elision of the and (because it's doing something like a if (x >= 0x80) { return; } path already)

[jeffhandley]

@tannergooding This looks good to me now. Can you re-review please?

[tannergooding]

@xtqqczze do you have any perf numbers?

The above diff report is showing no real change to the codegen (before and after are the same, just with after having an additional API now for Vector<T>)

This adds some complexity to the codebase that doesn't seem desirable long term.

[xtqqczze]

The above diff report is showing no real change to the codegen (before and after are the same, just with after having an additional API now for Vector<T>)

This is expected as the changes just create a unified implementation rather then having logic in different places doing the same thing.

I think this could be marked no-merge until #118583 is completed as the changes could then be considerably simplified.

[tannergooding]

Can you get numbers if you just use Narrow instead of creating a new NarrowNative?

[xtqqczze]

Wonder if that extra AND is a big deal..

Diff between Vector128.Narrow and Vector128.NarrowNative:

; Emitting BLENDED_CODE for generic X64 + VEX on Unix
+        vmovaps  xmm0, xmmword ptr [rsp+0x08]
+        vpackuswb xmm0, xmm0, xmmword ptr [rsp+0x18]
-        vbroadcastss xmm0, dword ptr [reloc @RWD00]
-        vpand    xmm1, xmm0, xmmword ptr [rsp+0x08]
-        vpand    xmm0, xmm0, xmmword ptr [rsp+0x18]
-        vpackuswb xmm0, xmm1, xmm0
         vmovups  xmmword ptr [rdi], xmm0
         mov      rax, rdi
+ 						;; size=19 bbWeight=1 PerfScore 7.25
- 						;; size=32 bbWeight=1 PerfScore 10.25

[tannergooding]

The question is more whether or not the potential perf difference there matters outside of micro-benchmarks

We're speculating that it's unlikely, particularly given newer hardware that has non saturating narrowing operations.

[xtqqczze]

Vector128.Narrow codegen looks worse on x86-64-v4:

; Emitting BLENDED_CODE for generic X64 + VEX + EVEX on Unix
+       vmovaps  xmm0, xmmword ptr [rsp+0x08]
+       vpackuswb xmm0, xmm0, xmmword ptr [rsp+0x18]
-       vmovaps  xmm0, xmmword ptr [rsp+0x08]
-       vpmovwb  xmm0, xmm0
-       vmovaps  xmm1, xmmword ptr [rsp+0x18]
-       vpmovwb  xmm1, xmm1
-       vmovlhps xmm0, xmm0, xmm1
        vmovups  xmmword ptr [rdi], xmm0
        mov      rax, rdi
+						;; size=19 bbWeight=1 PerfScore 7.25
-						;; size=35 bbWeight=1 PerfScore 13.25

[tannergooding]

That looks like a codegen issue. The two vectors are neighbors so it should be generating vmovups ymm0, ymmwork ptr [rsp+0x08]; vpmovmwb xmm0, ymm0 or similar

But that's also a bit beside the point. "Looks worse" doesn't actually mean "performs worse". What matters here is the real world perf for typical scenarios (not microbenchmarks). Without data showing otherwise, the presumption is that the slightly worse codegen doesn't matter and can be improved over time where it isn't doing the "right thing".

[tannergooding]

I'm going to say this is one we don't want to take.

We'd prefer to just use Narrow and then fix the codegen issue that was listed.

Thanks for the PR and locating the other codegen issue that we do want to resolve