Add basic support for TYP_MASK constants
#99743
Conversation
This is to support fixing JitOptRepeat (dotnet#94250). I was seeing failures in a Tensor test where `TYP_MASK` generating instructions were getting CSE'd. When OptRepeat kicks in and runs VN over the new IR, it wants to create a "zero" value for the new CSE locals. This change creates a `TYP_MASK` constant type, `simdmask_t`, like the pre-existing `simd64_t`, `simd32_t`, etc. `simdmask_t` is basically a `simd8_t` type, but with its own type. I expanded basically every place that generally handles `simd64_t` with `simdmask_t` support. This might be more than we currently need, but it seems to be a reasonable step towards making `TYP_MASK` more first-class. However, I didn't go so far as to support load/store of these constants, for example.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@jakobbotsch @tannergooding PTAL |
This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.
|
|
||
| bool operator==(const simdmask_t& other) const | ||
| { | ||
| return (u64[0] == other.u64[0]); |
There was a problem hiding this comment.
This is UB in C++ if u64 is not the active union members, but I guess simd64_t has the same behavior...
There was a problem hiding this comment.
It's UB per the spec. On the other hand, it's behavior that Clang, GCC, and MSVC all use themselves and which Windows, Linux, and MacOS all have various system level defines where similar is done. So it's something that's pretty heavily relied upon for all our targets
We could always switch to defining many different structs and using std::bit_cast or reinterpet_cast, but it doesn't buy us much in practice and the current definitions roughly match the official definitions used for __m128 and the related types used by Arm64
There was a problem hiding this comment.
I would rather be safe about stuff like this. It won't be the first time we have had bugs caused by similar cases of UB (e.g. #67669), and tracking these down is very painful. Getters/setters that use memcpy can be introduced to support the other typed accesses in a well-defined way.
| { | ||
| simdmask_t value = vnStore->ConstantValue<simdmask_t>(vnCns); | ||
|
|
||
| GenTreeVecCon* vecCon = gtNewVconNode(tree->TypeGet()); |
There was a problem hiding this comment.
Is VecCon really the right type to use here? This isn't a vector constant and I worry about other places potentially getting this wrong as they expect the type to only be TYP_SIMD*.
I'd have expected that we instead represent this is a new type, since its a completely separate kind of constant.
There was a problem hiding this comment.
Seems reasonable. I currently don't think we ever get here...
| printf("<0x%08x, 0x%08x>", vecCon->gtSimdVal.u32[0], vecCon->gtSimdVal.u32[1]); | ||
| break; |
There was a problem hiding this comment.
nit: I think this one would be better to display as a single 64-bit value. If we have a trivial way to format it as binary, that's all the better, as it is effectively a bitmask representing which elements are included or not.
There was a problem hiding this comment.
Makes sense. I wasn't sure why simd8 displayed as 2x32 either.
There was a problem hiding this comment.
For simd8, it's mostly for convenience. The most common data size used with vectors is 4-byte elements (float, int, and uint) so it makes the display more relevant for typical inputs while still being overall short.
This is to support fixing JitOptRepeat (#94250). I was seeing failures in a Tensor test where
TYP_MASKgenerating instructions were getting CSE'd. When OptRepeat kicks in and runs VN over the new IR, it wants to create a "zero" value for the new CSE locals.This change creates a
TYP_MASKconstant type,simdmask_t, like the pre-existingsimd64_t,simd32_t, etc.simdmask_tis basically asimd8_ttype, but with its own type. I expanded basically every place that generally handlessimd64_twithsimdmask_tsupport. This might be more than we currently need, but it seems to be a reasonable step towards makingTYP_MASKmore first-class. However, I didn't go so far as to support load/store of these constants, for example.