Share more of the TYP_MASK handling and support rewriting TYP_MASK operands in rationalization #103288
Conversation
…erands in rationalization
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@dotnet-policy-service rerun |
|
@dotnet/arm64-contrib FYI |
| @@ -99,6 +99,23 @@ void Rationalizer::RewriteNodeAsCall(GenTree** use, | |||
| // for intrinsics that get rewritten back to user calls | |||
| assert(!operand->OperIsFieldList()); | |||
|
|
|||
| if (varTypeIsMask(operand->TypeGet())) | |||
| { | |||
| #if defined(FEATURE_HW_INTRINSICS) | |||
There was a problem hiding this comment.
Instead wrap the if-condition in #ifdef FEATURE_MASKED_HW_INTRINSICS
There was a problem hiding this comment.
TYP_MASK and a lot of the surrounding support isn't under that feature switch.
If we were to centralize everything under that feature switch, I think it should be a separate PR. However, I'm also not convinced we need that as we didn't set any such flag up for XARCH.
There was a problem hiding this comment.
Have a separate PR is fine for me, but do we even see TYP_MASK if it is not FEATURED_HW_INTRINSICS?
There was a problem hiding this comment.
I don't believe we do today, but that doesn't mean it will never happen. varTypeIsMask returns constant false on unsupported platforms, so this just ensures any future scenarios handle this appropriately and will be optimized out as dead code otherwise.
| CorInfoType simdBaseJitType, | ||
| unsigned simdSize) | ||
| { | ||
| assert(varTypeIsMask(type)); |
There was a problem hiding this comment.
Do we need to pass a TYP_MASK for type every time we call gtNewSimdCvtVectorToMaskNode ? Since we only have one TYP_MASK, feels like we could just get rid of this parameter.
There was a problem hiding this comment.
It's here to support any future expansion desired. For example, it may be important to differentiate mask based on size.
src/coreclr/jit/hwintrinsic.cpp
Outdated
| @@ -1657,14 +1657,14 @@ GenTree* Compiler::impHWIntrinsic(NamedIntrinsic intrinsic, | |||
| // HWInstrinsic requires a mask for op2 | |||
| if (!varTypeIsMask(op2)) | |||
| { | |||
| retNode->AsHWIntrinsic()->Op(2) = gtNewSimdCvtVectorToMaskNode(retType, op2, simdBaseJitType, simdSize); | |||
| retNode->AsHWIntrinsic()->Op(2) = gtNewSimdCvtVectorToMaskNode(TYP_MASK, op2, simdBaseJitType, simdSize); | |||
There was a problem hiding this comment.
probably add an assert in gtNewSimdCvtVectorToMaskNode() that type == TYP_MASK?
There was a problem hiding this comment.
That assert already exists and is what caught the failure.
tannergooding
force-pushed
the
rewrite-mask
branch
from
151cc1a to
7241d3f
Compare
This resolves the issue introduced by #102702 which was discovered in #103094 (comment).
The bug was missed in the initial PR as the Arm64 handling for
mask <-> vectorconversions was a bit different from Xarch, and it wasn't expected to be able to encounter nodes in such a shape.