Add compile-time and run-time methods to obtain Vector<T> byte length - #130023
Add compile-time and run-time methods to obtain Vector<T> byte length#130023snickolls-arm wants to merge 11 commits into
Conversation
Renames getVectorTByteLength to getCompileTimeVectorTByteLength, which evaluates the size of Vector<T> using available ISA features. Adds getRuntimeVectorTByteLength which queries the actual size of Vector<T> using a cached class handle discovered during import. This can be used for example when trying to evaluate the size of the Tier0 frame. On Arm64 with SVE available, ISA features alone are not enough to predict the size of Vector<T>, motivating this change. The JIT can use getRuntimeVectorTByteLength to discover the SVE vector length in scenarios where an exact size is required, such as when generating OSR patchpoints.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@dotnet/arm64-contrib Please could I have a review? |
We can use this to access a handle to Vector<T> without explicitly seeing one in program metadata.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "424ec4f1cc75b120bdc1cc6c60685d4a348fe856",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "b2647f6fc4bf5a51e026f7dc880c9685b0947d13",
"last_reviewed_commit": "424ec4f1cc75b120bdc1cc6c60685d4a348fe856",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "b2647f6fc4bf5a51e026f7dc880c9685b0947d13",
"last_recorded_worker_run_id": "29682995741",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "424ec4f1cc75b120bdc1cc6c60685d4a348fe856",
"review_id": 4730587561
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: On Arm64 with SVE, the size of System.Numerics.Vector<T> cannot always be determined from compile-time ISA feature flags alone, because the scalable vector length is a runtime property. Some JIT scenarios (e.g. generating OSR patchpoints or SVE LoadNxVectorAndUnzip block layouts) require an exact size. The existing single getVectorTByteLength() helper conflated the compile-time and runtime notions of size, which is insufficient for that case.
Approach: The PR splits the concern into two clearly named helpers. getVectorTByteLength() is renamed to getCompileTimeVectorTByteLength() and, on Arm64 under the DEBUG-only JitUseScalableVectorT config, may now return the SIZE_UNKNOWN sentinel. A new getRuntimeVectorTByteLength() wraps it: when the compile-time answer is unknown, it queries the actual size via a new CLASSID_NUMERICS_VECTORT builtin class handle and getClassSize, guarded by assert(!IsAot()). Supporting plumbing adds the enum value in corinfo.h, resolves it in CEEInfo::getBuiltinClass via CoreLibBinder::GetClass(CLASS__VECTORT), and bumps the JIT-EE version GUID. Call sites are updated: the SVE unzip block-layout paths in gentree.cpp use the runtime helper, while purely compile-time paths (hwintrinsic.cpp, simd.cpp, lookupNamedIntrinsic) use the compile-time helper. A small refactor extracts the vector Count computation into evalVectorCount.
Summary: This is a well-scoped, correct JIT-EE interface addition with clear naming and documentation. The compile-time vs. runtime call-site split is applied consistently, and the JIT-EE GUID bump is present as required for the new getBuiltinClass class id. I have one non-blocking maintenance observation in the Detailed Findings regarding the managed enum mirror. Overall this looks good to merge.
Detailed Findings
-
CorInfoClassIdmanaged mirror not updated (minor / maintenance):CLASSID_NUMERICS_VECTORTis appended to the native enum insrc/coreclr/inc/corinfo.hand handled in the VM'sgetBuiltinClass, but the managed mirrorCorInfoClassIdinsrc/coreclr/tools/Common/JitInterface/CorInfoTypes.csand the correspondingswitchinCorInfoImpl.getBuiltinClass(src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs) are not updated. This is safe today: the new value is appended (so existing ordinals are unchanged), andgetRuntimeVectorTByteLengthonly callsgetBuiltinClass(CLASSID_NUMERICS_VECTORT)afterassert(!IsAot())on theSIZE_UNKNOWNpath, which itself is only reachable under the DEBUG-onlyJitConfig.JitUseScalableVectorT(). So neither the AOT/R2R compiler nor release builds will ever request this id. Still, keeping the managed enum in sync (and optionally implementing the case for the RyuJIT/crossgen managed path) would avoid a latentNotImplementedExceptionif a future change routes here, and keeps the two enum definitions from silently diverging. -
evalVectorCountrefactor is behavior-preserving: The extracted helper inimportercalls.cppreproduces the prior inline logic (getClassSize(clsHnd) / genTypeSize(simdBaseType)withGTF_ICON_SIMD_COUNT) exactly, and removing the now-unusedsimdSize/simdBaseTypelocals from the caller is correct. No functional concern.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 73.1 AIC · ⌖ 15 AIC · ⊞ 10K
|
@snickolls-arm, if it is ready for another review, please ping tanner. |
|
@tannergooding, this is ready for another review when you have a moment, thanks. |
| case CLASSID_NUMERICS_VECTORT: | ||
| result = CORINFO_CLASS_HANDLE(CoreLibBinder::GetClass(CLASS__VECTORT)); | ||
| break; |
There was a problem hiding this comment.
This is going to return an open generic type to the JIT. Does it make sense to query properties about the open generic type? Would it make more sense to return some particular instantiation, like Vector<byte>?
There was a problem hiding this comment.
I have found that it's enough to do this to cover all that I want to do, that is just call getClassSize. It works because the generic type, and all instantiations of, Vector<T> should have the same size, set by this function:
runtime/src/coreclr/vm/methodtablebuilder.cpp
Line 1189 in dad8c23
But yes, maybe this handle isn't useful for much beyond that. Am I leaving a pitfall by not providing an instantiated handle? Instantiating the class would still be fine for my use case.
Renames
getVectorTByteLengthtogetCompileTimeVectorTByteLength, which evaluates the size ofVector<T>using available ISA features.Adds
getRuntimeVectorTByteLengthwhich queries the actual size ofVector<T>using a cached class handle discovered during import.On Arm64 with SVE available, ISA features alone are not enough to predict the size of
Vector<T>, motivating this change. The JIT can usegetRuntimeVectorTByteLengthto discover the SVE vector length in scenarios where an exact size is required, such as when generating OSR patchpoints.