Repository navigation
Remove ShuffleNativeFallback - #134792
Remove ShuffleNativeFallback#134792BrzVlad wants to merge 3 commits into
Conversation
Direct ShuffleNative calls are expanded at the caller and preserve the underlying hardware behavior for out-of-range indices. Calling ShuffleFallback directly from the public method would make indirect invocations through reflection, delegates, or function pointers execute the deterministic fallback instead, which zeroes out-of-range elements and can disagree with direct calls in the same process. ShuffleNativeFallback existed to avoid that mismatch. The public method called a separate intrinsic helper, giving the JIT a second opportunity to emit ShuffleNative when compiling the public method itself. If expansion was unavailable, the helper was non-recursive, so its managed body could safely call ShuffleFallback. Replace this secondary intrinsic with the standard recursive intrinsic pattern. The public ShuffleNative methods recursively call themselves when their required hardware support is available. Recursive intrinsic calls are must-expand, so direct and indirect invocation use the same JIT expansion; when acceleration is unavailable, control reaches ShuffleFallback without recursion. Vector512 byte and sbyte shuffles require AVX512 VBMI even when Vector512 is otherwise hardware accelerated. Guard those overloads with `Avx512Vbmi.IsSupported`, allowing AVX512 targets without VBMI to use the managed fallback instead of failing recursive expansion. Mark these methods as `CompExactlyDependsOn` and `CompHasFallback` to disable compilation of the methods when the ISA is not known at R2R compilation time. This addresses issue of R2R code using the managed fallback, with no negative fixup recorded since negative dependencies from `IsSupported` are normally suppressed within SPC, while at runtime we could also have code using the `Avx512Vbmi` path. Remove NI_Vector_ShuffleNativeFallback and add reflection coverage that verifies direct and indirect ShuffleNative calls agree.
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
System.Runtime.Intrinsics.Tests failed with: System.InvalidProgramException: The JIT compiler encountered invalid IL code or an internal limitation.
| public static Vector256<byte> ShuffleNative(Vector256<byte> vector, Vector256<byte> indices) | ||
| { | ||
| #if !MONO | ||
| if (IsHardwareAccelerated) |
There was a problem hiding this comment.
So I don't think this alone is sufficient, particularly because of DOTNET_EnablePreferredVectorBitWidth=128 in which case Vector256.IsHardwareAccelerated is false but we still generate AVX2 code
I have some changes that I think work and it mostly involves replacing these with the correct Isa.IsSupported checks that the JIT keys codegen off of instead; then passing down mustExpand so we can make the right decision.
Is it alright if I push that change up to the PR here?
There was a problem hiding this comment.
Yes, feel free to push here, it will just require another reviewer. I was initially hoping the hardware accelerated check would be consistent with the generation of vectorized/fallback code. It's unfortunate that we have these special cases.
There was a problem hiding this comment.
Agreed. Some of its necessary for to ensure user testing and production scenarios work well, but I think the new changes handle it in a way that keeps it understandable and within what we support.
Use ISA support guards independent of preferred vector width and expand guarded recursive shuffles immediately with optimistic ISA support. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
2 participants
Direct
ShuffleNativecalls are expanded at the caller and preserve the underlying hardware behavior for out-of-range indices. CallingShuffleFallbackdirectly from the public method would make indirect invocations through reflection, delegates, or function pointers execute the deterministic fallback instead, which zeroes out-of-range elements and can disagree with direct calls in the same process.ShuffleNativeFallbackexisted to avoid that mismatch. The public method called a separate intrinsic helper, giving the JIT a second opportunity to emit ShuffleNative when compiling the public method itself. If expansion was unavailable, the helper was non-recursive, so its managed body could safely call ShuffleFallback.Replace this secondary intrinsic with the standard recursive intrinsic pattern. The public
ShuffleNativemethods recursively call themselves when their required hardware support is available. Recursive intrinsic calls are must-expand, so direct and indirect invocation use the same JIT expansion; when acceleration is unavailable, control reaches ShuffleFallback without recursion.Vector512 byte and sbyte shuffles require AVX512 VBMI even when Vector512 is otherwise hardware accelerated. Guard those overloads with
Avx512Vbmi.IsSupported, allowing AVX512 targets without VBMI to use the managed fallback instead of failing recursive expansion. Mark these methods asCompExactlyDependsOnandCompHasFallbackto disable compilation of the methods when the ISA is not known at R2R compilation time. This addresses issue of R2R code using the managed fallback, with no negative fixup recorded since negative dependencies fromIsSupportedare normally suppressed within SPC, while at runtime we could also have code using theAvx512Vbmipath.Remove NI_Vector_ShuffleNativeFallback and add reflection coverage that verifies direct and indirect ShuffleNative calls agree.