Use an explicit allow list for SIMD types in getTypeLayout - #131479
Conversation
The intrinsic check was a deny form: every [Intrinsic] type in System.Numerics and System.Runtime.Intrinsics was reported to the JIT as an opaque SIMD node, with the decimal floating-point types carved back out. That only held while every intrinsic type in those namespaces happened to be SIMD, so each new non-SIMD intrinsic type needs another carve-out. Name the SIMD types explicitly instead, mirroring the set recognized by Compiler::getBaseTypeAndSizeOfSIMDType. Matrix3x2/Matrix4x4 now fall out of the set as well; the JIT already returned TYP_UNDEF for them, so their simdTypeHnd was never usable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 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: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR tightens getTypeLayout’s SIMD “primitive” detection by replacing a broad namespace-based intrinsic check with an explicit allow list of SIMD types, ensuring non-SIMD [Intrinsic] types in System.Numerics / System.Runtime.Intrinsics have their fields enumerated and reported to the JIT.
Changes:
- Add a VM-side
IsSimdIntrinsicType(MethodTable*)helper and use it to decide when to setsimdTypeHndinCEEInfo::GetTypeLayoutHelper. - Add a managed (crossgen2/ILCompiler)
IsSimdIntrinsicType(MetadataType)helper and use it for the analogousGetTypeLayoutHelperlogic. - Remove the previous “deny-form” namespace-only SIMD detection (and associated decimal carve-out) from both implementations.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.cpp | Introduces an explicit SIMD allow list helper and uses it to drive simdTypeHnd in getTypeLayout. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Mirrors the VM behavior for AOT/tooling by introducing the same allow-list concept using existing vector type helpers. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
CC. @MichalStrehovsky, this does the explicit filtering to avoid any potential future issues, only skipping for the types we definitely know are safe (because they map to |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Is the purpose of this basically a compile-time perf optimization? I assume that would mean it's sufficient to match a subset of types recognized by Compiler::getBaseTypeAndSizeOfSIMDType that has many fields. If all of these have many fields, than exact mirror makes sense. If only some of these, we could potentially delete some lines of code. I'll leave this up to you, I'll not pretend I know about SIMD!
@jakobbotsch added this particular path as part of the promotion feature and so I'd defer to him as the expert for that particular portion. As far as I understand it, however, it is essentially just part of ensuring that these are viewed as enregistered primitives and stopping the consideration of the underlying fields that they are made up of. -- It's actually possible that this may be leaving some on the table since V256/V512, for example, aren't such cases on platforms like WASM/Arm64, but that's a pre-existing limitation, not something newly introduced. |
Follow-up to #131345 (comment).
The intrinsic check in
getTypeLayoutwas a deny form: every[Intrinsic]type inSystem.NumericsandSystem.Runtime.Intrinsicswas reported to the JIT as an opaque SIMD node, with the decimal floating-point types carved back out viaIsDecimalFloatingPointOrHasDecimalFloatingPointFields. That only held while every intrinsic type in those namespaces happened to be SIMD, so each new non-SIMD intrinsic type needs another carve-out.This names the SIMD types explicitly instead, via a new
IsSimdIntrinsicTypehelper on both the native VM and the managed (crossgen2 / ILCompiler) side, mirroring the set recognized byCompiler::getBaseTypeAndSizeOfSIMDType:System.Runtime.Intrinsics:Vector64<T>,Vector128<T>,Vector256<T>,Vector512<T>System.Numerics:Vector<T>,Vector2,Vector3,Vector4,Plane,QuaternionThe managed helper reuses the existing
VectorFieldLayoutAlgorithm.IsVectorTypeandVectorOfTFieldLayoutAlgorithm.IsVectorOfTTyperather than duplicating those name checks.IsDecimalFloatingPointOrHasDecimalFloatingPointFieldsis untouched -- it's still used for the by-value marshal restriction indllimport.cpp,mlinfo.cpp,classlayoutinfo.cpp, andMarshalHelpers.cs.Matrix3x2/Matrix4x4fall out of the set as well. They're[Intrinsic]and live inSystem.Numerics, so the old namespace-only test caught them, butgetBaseTypeAndSizeOfSIMDTypereturnsTYP_UNDEFfor them, so theirsimdTypeHndwas never usable. Their fields are now enumerated like any other struct:TryPromoteValueClassAsPrimitiverejected aMatrixfield before (SIMD lookup fails, thennumFields == 0 != 1) and rejects it now (simdTypeHnd == null, thennumFields == 6 or 16 != 1), so no promotion can be lost.TryPromoteStructVarhas a1 + MAX_NumOfFieldsInPromotableStruct * 2 == 9node budget; aMatrix4x4field overflows it intoPartial->return false, the same outcome as before.ClassLayout::GetNonPaddingtreats asimdTypeHndnode as entirely non-padding. Matrices have no padding, andSegmentList::Addmerges the adjacent per-field segments, so the resulting map is identical.Matrixtypes no longer hit theNeedsTypeLayoutCheck->Failurebail and take the ordinary field-enumeration path. Their fields are primitivefloats, whichCheckTypeLayouthandles; thegetClassSizefixup concern applies only to types reported viasimdTypeHnd.No JIT-EE GUID bump -- the interface signature and
CORINFO_TYPE_LAYOUT_NODEare unchanged.Deliberately out of scope:
MethodTable::ClassifyEightBytes*andSystemVStructClassificatoralso do intrinsic-type name checks, but against a deliberately smaller set (onlyVector64/128/256/512<T>andVector<T>--Vector2/3/4,Plane, andQuaternionare passed in registers), so they can't share this helper.Validation
SuperPMI can't cover this -- it replays recorded
getTypeLayoutresponses, so a VM-side change is invisible to asmdiffs. Validated with a real build instead:build.cmd clr -rc checked-- 0 warnings, 0 errors. BothILCompiler.RyuJitandILCompiler.ReadyToRunrelink, so both#if READYTORUNarms compile.JitDumpover wrapper structs for all 15 affected types (10 SIMD +Matrix3x2/Matrix4x4+Decimal32/64/128), before vs. after, address-normalized: 0 differing lines out of 83,382. This confirms empirically that theMatrixdelta is inert, including theMatrix3x2case that fits within the node budget.JIT/SIMD: 116/116 passed.JIT/Directed/StructPromote+JIT/Directed/physicalpromotion: 9/9 passed.Note
This PR description and the accompanying code changes were drafted with GitHub Copilot.