-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Refactor GenTreeJitIntrinsic to store the base type for SIMD operations as a var_types.
#121251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
2a907d6 to
7436196
Compare
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.
Pull Request Overview
This PR refactors GenTreeJitIntrinsic to store the base type for SIMD operations as var_types instead of CorInfoType, enabling support for SIMD operations on types not part of the CIL. The changes include:
- Renaming
simdBaseJitTypetosimdBaseTypethroughout the codebase - Converting
CorInfoTypeparameters tovar_typesin SIMD-related functions - Introducing new helper functions like
getBaseTypeForPrimitiveNumericClassandgetBaseTypeAndSizeOfSIMDType - Maintaining backward compatibility through new
getCorBaseTypeXXfunctions
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| valuenum.h | Updated signature of VNForSimdType to use var_types instead of CorInfoType |
| valuenum.cpp | Changed parameter types in VNForSimdType and updated call sites |
| simd.cpp | Added new getBaseTypeAndSizeOfSIMDType function returning var_types, refactored from original implementation |
| rationalize.cpp | Updated all SIMD intrinsic operations to use var_types for base type tracking |
| optimizemaskconversions.cpp | Changed mask conversion tracking to use var_types |
| morph.cpp | Updated HWIntrinsic morphing to use var_types throughout |
| lowerxarch.cpp | Converted all SIMD lowering operations to use var_types |
| lowerarmarch.cpp | Updated ARM-specific SIMD lowering to use var_types |
| lower.h | Changed function signature for InsertNewSimdCreateScalarUnsafeNode |
| lower.cpp | Updated implementation to use var_types |
| lclvars.cpp | Changed local variable promotion to use var_types |
| lclmorph.cpp | Updated local morphing to use var_types |
| importervectorization.cpp | Changed vectorization import to use var_types |
| importercalls.cpp | Updated intrinsic call handling to use var_types with JitType2PreciseVarType conversions |
| importer.cpp | Changed impNormStructType signature and implementation |
| hwintrinsicxarch.cpp | Updated x86/x64 intrinsic handling with extensive conversions from CorInfoType to var_types |
| hwintrinsiccodegenxarch.cpp | Updated codegen to use var_types |
| hwintrinsicarm64.cpp | Converted ARM64 intrinsic handling to use var_types throughout |
|
|
||
| if (simdBaseJitType == CORINFO_TYPE_UNDEF) | ||
| var_types GenTreeJitIntrinsic::GetSimdBaseTypeAsVarType() const |
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.
nit: This name is a bit confusing.
I also thought we had something which covered this normalization already. genActualType isn't quite it, since it does smallTypes -> TYP_INT as well, but I was thinking we already had a helper for return varTypeIsSmall(x) ? x : genActualType(x);
Perhaps @EgorBo or @jakobbotsch remember?
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.
Depending on how this is used, genActualType itself might be the "better" choice.
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.
I agree this is confusing. What is this used for?
If this is used to get the type to use for loads or stores of elements then perhaps something like GetIndirTypeForElement? (It should have comment about it as well.)
Perhaps @EgorBo or @jakobbotsch remember?
I don't think we have anything that does this exact operation.
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.
I think this is used in places where the jit wants the base type for canonicalization purposes (TYP_INT or TYP_LONG) and does not want to differentiate between TYP_INT and TYP_UINT etc.
This was for places where JitType2VarType was used and not JitType2PreciseVarType.
|
cc. @dotnet/jit-contrib, @EgorBo, @jakobbotsch for secondary review |
This PR adjust
GenTreeJitIntrinsicto save the base type of SIMD operation as avar_typesand notCorInfoType. This is done to allow SIMD operations on types that are not part of the CIL.Some notes on the changes:
simdBaseJitTypehas been renamed tosimdBaseTypeand most flows that previous had both asimdBaseJitTypeandsimdBaseTypenow only use thesimdBaseType.simdBaseTypeshould now refer to the type returned viaJitType2PreciseVarType.JitType2VarType.gtAuxiliaryJitTypehas been left unchanged. Some new functions have been introduced calledgetCorBaseTypeXXthat keep the original behavior.If we decide to also change
gtAuxiliaryJitTypeI can make the changes and refactor things a bit more.Current opening PR as draft to get a test run.