JIT: Avoid incorrect TYP_UINT retyping for ToScalar #71372
Conversation
TYP_UINT is only meaningful for certain operations in the JIT and nodes can/should never produce values of this type. Fix dotnet#71360
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsTYP_UINT is only meaningful for certain operations in the JIT and nodes Fix #71360
|
|
The logic in |
|
cc @dotnet/jit-contrib PTAL @kunalspathak |
| if (varTypeIsSmall(type)) | ||
| { | ||
| // Normalize 'type', it represents the item that we will be storing in the Outgoing Args | ||
| type = TYP_INT; |
There was a problem hiding this comment.
Just that I understand, was there an inconsistency in the way we set type here to TYP_INT but not doing the same in ToScalar()?
There was a problem hiding this comment.
I would say the bug is in ToScalar alone, typing a node as TYP_UINT (or TYP_ULONG) is invalid IR. These types are meaningless except for casts (and maybe some SIMD base types).
Under the assumption that we never see TYP_UINT or TYP_ULONG typed nodes this logic here is equivalent to genActualType.
There was a problem hiding this comment.
Got it. So is the assert reported in #71360 is the only one that checks for this or do we have other places we make sure that type is never one of TYP_UINT or TYP_ULONG?
There was a problem hiding this comment.
Most places in the JIT should be dealing with genActualType that would work correctly even for TYP_UINT/TYP_ULONG. I'm not sure if we have good checking in the places that wouldn't work correctly, but it is hard to know which places that would be. One place is IntegralRange::ForNode that would hit an unreachable().
I've added an item to #12121 about it.
There was a problem hiding this comment.
and maybe some SIMD base types
Its most SIMD base types in practice. The SIMD base type should always be the exact type required for the underlying instruction. That includes correctly tracking the sign and size. This is important because there are many SIMD instructions that differ based on size/type and will do the wrong thing otherwise.
It was definitely a bug that the gtType wasn't the right thing, however.
3 participants
TYP_UINT is only meaningful for certain operations in the JIT and nodes
can/should never produce values of this type.
Fix #71360