Fix SIMD intrinsics handling in crossgen2 #27853
Conversation
Crossgen2 was compiling methods that call SIMD intrinsics (System.Numerics.Vector<T>). This is not correct, as the size of the vector is a runtime specific detail - e.g. when running on devices without SSE2 support, the size is 4 and when running on devices with SSE2 support, the size is 8. This fixes runtime errors in 6 coreclr pri 0 tests.
| @@ -697,6 +697,13 @@ private uint getMethodAttribsInternal(MethodDesc method) | |||
| result |= CorInfoFlag.CORINFO_FLG_FINAL; | |||
| } | |||
|
|
|||
Create a IsVectorOfT method and use it at all places where we were previously checking the namespace and type name of Vector<T>
|
Is this a regression? GetLayoutAlgorithmForType should make sure that we never come up with a valid layout for these types (the size will never be 4 or 8, as the description alludes to). When jit asks for the layout, the type system says the size is indeterminate, and we end up throwing RequiresRuntimeJit. |
|
I don't think it is a regression, the related tests (e.g JIT/SIMD/VectorDot_r) were failing for as long as I remember. |
|
But let me check why the stuff you've mentioned doesn't kick in |
|
@MichalStrehovsky where would you expect the exception to be thrown? I've set a breakpoint at the GetLayoutAlgorithmForType in the branch that returns the layout for the Vector and after hitting it, I've stepped out of that method until I've reached the JIT code. I haven't seen any exception throwing around the code path. |
|
Tomáš can probably point you to it better - I'm on the phone only these days. Look for |
|
Actually, maybe the case the tests were hitting is special and different. The Main method contains the following lines: if (VectorDotTest<float>.VectorDot(3f, 2f, 6f * Vector<float>.Count) != Pass) returnVal = Fail;
if (VectorDotTest<double>.VectorDot(3d, 2d, 6d * Vector<double>.Count) != Pass) returnVal = Fail;
if (VectorDotTest<int>.VectorDot(3, 2, 6 * Vector<int>.Count) != Pass) returnVal = Fail;
if (VectorDotTest<long>.VectorDot(3, 2, (long)(6 * Vector<long>.Count)) != Pass) returnVal = Fail;So only the Count method is being called, which is a static method and also an intrinsic. So maybe computing |
|
I can see that even before my change, we were not compiling e.g. the VectorDotTest method with the following message: |
| @@ -106,5 +106,10 @@ public static bool IsVectorType(DefType type) | |||
| type.Name == "Vector128`1" || | |||
| type.Name == "Vector256`1"); | |||
| } | |||
|
|
|||
| public static bool IsVectorOfTType(DefType type) | |||
There was a problem hiding this comment.
For #23899, I had added this as InteropTypes.IsSystemNumericsVectorT, as that appears to be where a fair number of the other IsType checks live
There was a problem hiding this comment.
I am not sure which location is better. Your methods are also a bit different as they require the TypeSystemContext parameter. So maybe keeping both versions makes sense. There was already the IsVectorType here, so adding the IsVectorOfTypeT to the same place felt ok.
There was a problem hiding this comment.
InteropTypes only handles types interesting for interop (as the name implies). Up until Tanner's change it was exclusively used in generating/figuring out marshalling. Crossgen2 doesn't have a central MscorlibBinder equivalent (as it exists in CoreCLR).
We place these as close to the usage as possible. Centralized locations tend to end up with orphaned entries.
Crossgen2 was compiling methods that call SIMD intrinsics
(System.Numerics.Vector). This is not correct, as the size of the
vector is a runtime specific detail - e.g. when running on devices
without SSE2 support, the size is 4 and when running on devices with
SSE2 support, the size is 8.
So methods that call SIMD intrinsics should not be crossgened.
This fixes runtime errors in 6 coreclr pri 0 tests.