Arm64/Sve: Implement AbsoluteCompare* and Compare* APIs #104464
Conversation
ghost
added
|
Note regarding the |
1 similar comment
|
Note regarding the |
|
@dotnet/arm64-contrib |
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
| getLowVectorOperandAndCandidates(intrin, &lowVectorOperandNum, &lowVectorCandidates); | ||
| } | ||
|
|
||
| if ((intrin.id == NI_Sve_ConditionalSelect) && (intrin.op2->IsEmbMaskOp()) && |
There was a problem hiding this comment.
This was dead code
There was a problem hiding this comment.
Yeah, looks like we handle embedded masks just above this snippet, right here.
| } | ||
|
|
||
| [method: MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private void ConditionalSelectScenario_TrueValue({Op1VectorType}<{Op1BaseType}> mask, {Op1VectorType}<{Op1BaseType}> op1, {Op1VectorType}<{Op1BaseType}> op2, {Op1VectorType}<{Op1BaseType}> falseOp) |
There was a problem hiding this comment.
There are a couple of cases in this template where the second operand uses Op1BaseType instead of Op2BaseType. This shouldn't cause any problems for now, since the APIs you're adding use operands of the same type. Should we fix this before checking the new template in, or should we wait to fix this once we add new APIs with different operand types (like what we've been doing for the last few)? I'm fine with either approach.
There was a problem hiding this comment.
yes, perhaps it needs a audit for similar templates, so will probably do this on need basis.
|
Test changes LGTM, too. |
| if (HWIntrinsicInfo::ReturnsPerElementMask(node->GetHWIntrinsicId())) | ||
| { | ||
| nodeType = TYP_MASK; | ||
| } |
There was a problem hiding this comment.
Shouldn't this just be node->TypeGet() 100% of the time?
There was a problem hiding this comment.
yes, will change it in follow-up PRs.
| SingleTypeRegSet candidates = lowVectorOperandNum == 2 ? lowVectorCandidates : RBM_NONE; | ||
|
|
||
| if (intrin.op2->gtType == TYP_MASK) | ||
| if (intrin.op2->OperIsHWIntrinsic(NI_Sve_ConvertVectorToMask)) |
There was a problem hiding this comment.
Why does this change the register set to MASK but regular TYP_MASK nodes do not?
There was a problem hiding this comment.
Its the same reason I mentioned in #102611 (comment)
3 participants
Some changes are from @mikabl-arm contributions in #102611.
All stress tests are passing.