Map SIMD types directly from class names #121489
Conversation
The current implementation derives a size based on the identified SIMD type, and then uses the size to derive the node type. It should instead directly derive the node type from the identified SIMD type, because some SIMD types will not have a statically known size, and this size may conflict with other SIMD types.
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| // product. | ||
| // | ||
| CorInfoType Compiler::getBaseJitTypeAndSizeOfSIMDType(CORINFO_CLASS_HANDLE typeHnd, unsigned* sizeBytes /*= nullptr */) | ||
| var_types Compiler::getSIMDType(CORINFO_CLASS_HANDLE typeHnd, CorInfoType* baseType) |
There was a problem hiding this comment.
I'm not sure I understand the motivation for this over just setting *sizeBytes = UINT32_MAX or *sizeBytes = 0 in the case it's variable sized.
We still often need a size and so this just seems like extra code churn over directly handling "unknown size". -- Most of the callers of this replacement method just end up doing genTypeSize(simdType) after all, so it particularly seems like "extra steps" from what we already have.
There was a problem hiding this comment.
I have kept the original function as a wrapper around this function, so it's still possible to use it in scenarios that know they want a size. Most of the function body is the same but it didn't diff very well, but it's not a replacement.
I'm trying to deal with the ambiguity in JIT mode when sizeof(Vector<T>) == 16. This results in the type resolving to TYP_SIMD16, which will always produce NEON code so we can't run 128-bit SVE. We cannot unconditionally map size 16 => unknown when JitUseScalableVectorT=1 because this will cause us to generate SVE for Vector128 when we should be generating NEON. So it's important to know whether we were originally dealing with Vector<T> or Vector<128> when choosing the type.
There was a problem hiding this comment.
I think we want Vector<T> to always produce "size unknown" if SVE is enabled.
Any type of opportunistic lightup when SVE happens to be 128-bits should be explicit and done via a set of alternative checks, such as a pass which normalizes such locals to TYP_SIMD16 and then inserts a "conversion" from one to the other (which can later be elided as zero cost).
This would best match what real user code would end up having to do and with what AOT will have to do in typical scenarios.
There was a problem hiding this comment.
-- This is notably similar to the approach we landed on for TYP_MASK as well.
Where the JIT IR naturally does the same thing that user code equates to. We then have a set of conversions and optimizations that allow the JIT to do "better things" opportunistically based on the IR shape and invariants detected.
There was a problem hiding this comment.
I think we want Vector to always produce "size unknown" if SVE is enabled.
OK sure, this will solve the ambiguity as well. I'll try refactoring it this way.
I'm still trying to understand the preference for using a sentinel value in simdSize when this information is already encoded in the type? If type <==> size then why pass around both and not just use one of them to derive the other? Does this not hold somewhere? Or are we moving away from TYP_SIMD16/32/... towards a single TYP_SIMD with the size as a subtype? Just making sure I've got the right the design intentions in mind for future.
There was a problem hiding this comment.
I'm still trying to understand the preference for using a sentinel value in simdSize when this information is already encoded in the type?
We've historically always gotten the size and then derived the type from the size, which is why getBaseJitTypeAndSizeOfSIMDType returns the base type (i.e. the type of the T given Vector<T>) and additionally the size today.
Or are we moving away from TYP_SIMD16/32/... towards a single TYP_SIMD with the size as a subtype?
This likely wouldn't be feasible because there are places that only have space for a TYP_* entry, such as locals.
There are places that need the size, places that need the type, and places that need both. It really all depends on the IR, where the data is needed, and how the data is used.
It doesn't really matter whether we get the size and then derive the type or get the type and then derive the size. Both are functionally equivalent. However, today we are doing the former (getting the size and then deriving the type) so preserving that behavior will result in a smaller amount of code churn and less risky PR.
There was a problem hiding this comment.
This makes sense, thanks for clarifying.
There was a problem hiding this comment.
The approach you're suggesting makes this patch redundant, so I'll move on to the next patch and we can close this. I'll introduce SIZE_UNKNOWN and TYP_SIMD at the same time. I'll need to introduce the config variable as well to avoid breaking the mainline code.
2 participants
The current implementation derives a size based on the identified SIMD type, and then uses the size to derive the node type. It should instead directly derive the node type from the identified SIMD type, because some SIMD types will not have a statically known size, and this size may conflict with other SIMD types.