Fix Vector256.IsHardwareAccelerated in R2R binaries #65351
Conversation
- If a function had a dependency on Vector256<T> but not on any Avx2 functionality then we would fail to produce an R2R image using a debug version of the compiler, and would generate an incorrect function where the result of calling Vector256.IsHardwareAccelerated would produce the wrong result. - To fix this we now annotate the VectorXXX instruction sets so that if there is a negative depenedency on them during R2R generation, that the function is not available for use if the underlying actual instruction set is available to use at runtime. - Adjust test Runtime_34587 to test instruction set manner in a way that allows testing crossgen2 instruction set variation - Move instruction set issupported checks into standalone functions to isolate the tests in the JIT/Crossgen2 compiler to better see exactly what is happening
ghost
added
the
davidwrighton
requested review from
tannergooding and
trylek
and removed request for
MichalStrehovsky
| switch(input) | ||
| { | ||
| case InstructionSet.X86_Vector128: return InstructionSet.X86_SSE; | ||
| case InstructionSet.X86_Vector256: return InstructionSet.X86_AVX; |
There was a problem hiding this comment.
should Vector<T> be normalized on the same handling?
Should there be a comment covering that:
- The ABI handling for Vector128/256 requires Sse/Avx, this is what's required for them to be passed around as
__m128and__m256, respectively - The
Vector128/256.IsHardwareAcceleratedproperty only returnstrueunder Sse2/Avx2 (this is when users can depend on most functions exposed to be SIMD accelerated)
There was a problem hiding this comment.
Abi handling is actually separate from this entirely and is handled by an abi stability protection construct which works independently of all of this. (Mostly as the ABI for both Vector128 and Vector256 doesn't match the native vector ABI on Windows, and there was a desire at one point to unify on the actual official ABI. This has not yet happened.
There was a problem hiding this comment.
So is this check only for IsHardwareAccelerated then?
There was a problem hiding this comment.
Just trying to understand what these particular checks line up with.
We support efficiently passing and some acceleration for Vector128<T> under SSE and Vector256<T> under AVX.
However, IsHardwareAccelerated will only return true under SSE2 and AVX2, respectively.
There was a problem hiding this comment.
This lines up with the Vector128/Vector256 api usage in the JIT. Effectively, when the jit determines that it cannot use a VectorXXX api it will call up, and notify crossgen that It has determined that if VectorXXX is useable at runtime via any intrinsics the generated code cannot be used. At that stage crossgen needs to encode that one of the instruction sets is not permitted, as it cannot encode that the VectorXXX intrinsics are not available. So, what it does it encode that the minimum required instruction set for the VectorXXX intrinsics is not available. Handling of IsHardwareAccelerated is actually a side-effect of this. In the encoding there are actually more states for how this works than is pleasant to contemplate. For instance, the options available to crossgen for encoding a method which calls Vector256<T>.IsHardwareAccelerated are:
- Generate a method where IsHardwareAccelerated returns false, and mark that if Avx is supported, then the method cannot be used. (In this case the JIT will specify that support for the Vector256 instruction set is required to be disabled)
- Generate a method where IsHardwareAccelerated returns false, and mark that if Avx2 is supported, then the method cannot be used, and also mark that Avx must be supported to use the method. (In this case the JIT will specify that support for the Vector256 instruction set is required to be enabled, but Avx2 support is required to be disabled.)
- Generate a method where IsHardwareAccelerated returns true, and mark that if Avx2 is not supported then the method cannot be used. (In this case the JIT will specify that support for the Vector256 instruction set is required to be enabled, and Avx2 support is required to be enabled.)
The problem that this bug solves is that while the previous logic correctly handled cases 2 and 3, it did not handle case 1. (What would happen is that the JIT would notify crossgen that Vector256 could not be used, and be we would fail to record that there was an instruction set restriction on the generated code.)
| @@ -80,7 +81,9 @@ public static int Main() | |||
| bool succeeded = true; | |||
|
|
|||
| succeeded &= ValidateArm(); | |||
| Console.WriteLine($"ValidateArm:{succeeded}"); | |||
There was a problem hiding this comment.
nit: This will only correctly report the first failure. If say, Arm fails it will also report all subsequent validations as failing.
There was a problem hiding this comment.
nit: If you fix this, might be nice to also add a space between : and {succeeded} to help with readability
Fix #63818