-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Updating the VM to no longer treat the SIMD HWIntrinsic types as HFA or MultiReg structs #15942
Conversation
…or MultiReg structs.
FYI. @CarolEidt, @jkotas, @fiigii, @sdmaclea |
I initially tried to do this in the JIT, but there are a lot of assumptions for any type which has had the |
Logged https://github.com/dotnet/coreclr/issues/15943, to track adding the proper ABI support for these types. |
test Windows_NT x64 Checked jitincompletehwintrinsic test Windows_NT x86 Checked jitincompletehwintrinsic test Ubuntu x64 Checked jitincompletehwintrinsic test OSX10.12 x64 Checked jitincompletehwintrinsic |
ARM64 may be different, but it would be nice if it was the same. Maybe we should have/require a cast operator to ABI compatible type to support interop with C++ ABI. |
@sdmaclea. The Procedure Call Standard for the |
All failures are known issues unrelated to this change (https://github.com/dotnet/coreclr/issues/15924 and https://github.com/dotnet/coreclr/issues/15848). |
Updated to prevent the SIMD hardware intrinsic types from being loaded during crossgen, as per https://github.com/dotnet/coreclr/issues/15943#issuecomment-359144600 |
src/vm/methodtablebuilder.cpp
Outdated
// Disable AOT compiling for the SIMD hardware intrinsic types. These types require special | ||
// ABI handling as they represent fundamental data types (__m64, __m128, and __m256) and not | ||
// aggregate or union types. See https://github.com/dotnet/coreclr/issues/15943 | ||
COMPlusThrow(kTypeLoadException, IDS_EE_HWINTRINSIC_NGEN_DISALLOWED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas @CarolEidt If SIMD types are disabled for ABI, how can we enable HW intrinsics for crossgen (or other AOT platforms) in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One we have the right stable ABI for the Vector64/128/256 types, we can re-enable it for crossgen/AOT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tannergooding Could you please update the comment to capture this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to additionally include the following comment: Once they are properly handled according to the ABI requirements, we can remove this check and allow them to be used in crossgen/AOT scenarios.
test Ubuntu x64 Checked jitincompletehwintrinsic test OSX10.12 x64 Checked jitincompletehwintrinsic |
test Windows_NT x64 Checked jitincompletehwintrinsic test Windows_NT x86 Checked jitincompletehwintrinsic |
👍 I see them referred to as Short Vectors in AAPCS. I had ignored them, because @jkotas had indicated they were not supported. Sounds like we need to fully support as part of HW intrinsics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look reasonable to me but @jkotas should be the one to approve these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Prior to #15897, these (
Vector64<T>
,Vector128<T>
, andVector256<T>
) were explicitly sized types with 0 introduced fields, so they would fall out of these checks and would not be treated as HFA or MultiReg structs.After the change, these began being treated as 2-field Integer aggregates which caused them to be passed around incorrectly.
After discussion with @CarolEidt, we agreed that disabling the HFA/MultiReg handling for these types was the best/easiest solution for the time being.
At some point in the future, we should add in the proper handling for these types and treat them as the
__m64
,__m128
, and__m256
scalar (e.g. non aggregate and non union) types which are defined in the System V and Windows ABIs.