JIT ARM64-SVE: Add AbsoluteCompare* APIs #102611
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kunalspathak @dotnet/arm64-contrib @a74nh |
|
Note regarding the |
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
Thanks. What about stress test results? |
kunalspathak
requested changes
May 23, 2024
| if (intrin.op3->IsVectorZero()) | ||
| if (!instrIsRMW) | ||
| { | ||
| assert(intrin.op3->IsVectorZero()); |
There was a problem hiding this comment.
intrin.op3 should be nullptr because there are just 2 operands involved.
There was a problem hiding this comment.
In this context intrin is Sve_ConditionalSelect, not Sve_AbsoluteCompare*.
| @@ -3004,6 +3004,16 @@ | |||
| ("SveSimpleVecOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_BooleanNot_uint", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "BooleanNot", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt32", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()", ["ValidateIterResult"] = "Helpers.BooleanNot(firstOp[i]) != result[i]", ["GetIterResult"] = "Helpers.BooleanNot(leftOp[i])"}), | |||
| ("SveSimpleVecOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_BooleanNot_ulong", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "BooleanNot", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt64", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt64", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt64()", ["ValidateIterResult"] = "Helpers.BooleanNot(firstOp[i]) != result[i]", ["GetIterResult"] = "Helpers.BooleanNot(leftOp[i])"}), | |||
|
|
|||
| // Sve mask | |||
There was a problem hiding this comment.
Suggested change
| // Sve mask |
Thanks, missed those. There are several errors actually. I'll take a look. |
|
After 4b5a0f8 the stress tests now pass successfully. The issue arises when emitting To address this issue, the commit 4b5a0f8 conditionally changes the type of the We've also discussed two other approaches with @a74nh and @SwapnilGaikwad . At the moment
I haven't tested these approaches yet. Stress tests' results: |
This sounds reasonable to me. By doing this, can we add extra test coverage? For e.g. can we land up with https://docsmirror.github.io/A64/2023-06/sel_p_p_pp.html . Or what happens when we use something like this: ConditionalSelect(AbsoluteCompare(left, right), left, right));
LoadVector(AbsoluteCompare(left, right), address)); |
|
@kunalspathak , thank you for the review. I'm happy to add additional test for the APIs.
Are you asking for a tests that checks the emitted instructions for
Do you want these two to be added for all APIs that use |
I think this falls in the category of #103078, so we can probably skip doing it.
I think we should do it at least for one of the |
|
Hello @kunalspathak , I've added the requested tests. Please let me know if I interpreted something incorrectly. |
mikabl-arm
force-pushed
the
AbsoluteCompare
branch
from
613054c
to
629bb75
Compare
| @@ -1311,9 +1311,14 @@ GenTree* Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) | |||
| GenTree* trueMask = comp->gtNewSimdAllTrueMaskNode(simdBaseJitType, simdSize); | |||
| GenTree* trueVal = node; | |||
| GenTree* falseVal = comp->gtNewZeroConNode(simdType); | |||
| var_types nodeType = simdType; | |||
| if (HWIntrinsicInfo::ReturnsPerElementMask(node->GetHWIntrinsicId())) | |||
There was a problem hiding this comment.
I need to test this out to make sure this is correct. I will pull your branch once #103288 is merged.
There was a problem hiding this comment.
I am seeing the test DOTNET_TieredCompilation=0:
Assert failure(PID 18916 [0x000049e4], Thread: 28152 [0x6df8]): Assertion failed 'unreached' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleBinaryOpTest__Sve_AbsoluteCompareGreaterThan_float:ConditionalSelect_MethodMask():this' during 'Generate code' (IL size 109; hash 0x5f5a6ecd; FullOpts)
File: D:\git\runtime\src\coreclr\jit\emitarm64sve.cpp:4396
Image: D:\kpathak\Core_Root_absolutecompare\Core_Root\corerun.exe
This is missing a piece where if the cndSelNode is of type TYP_MASK, then the falseValue should also be TYP_MASK. Today, we do not support ZeroConNode of TYP_MASK, so I was thinking of wrapping falseVal with ConvertVectorToMask(), but for some reason that was failing in LIR validation that I didn't get a chance to verify. I will look tomorrow.
There was a problem hiding this comment.
@a74nh, @mikabl-arm - I have fixed code to handle falseValue of TYP_MASK. PTAL.
Stress tests are passing: https://gist.github.com/kunalspathak/95ddd4913a6642bcbda7ddff9c33f752
The failure case RunLoadMask is because we are not preserving p0 across call (known problem).
mikabl-arm
force-pushed
the
AbsoluteCompare
branch
from
629bb75
to
8375ac5
Compare
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
|
Did you verify the test after latest merge? I see this failure on |
…nalSelect in another ConditionalSelect if required
|
Thank you for catching this, @kunalspathak ! Indeed, looks like I have missed something after adding the two requested new tests. I'll do my best to be more thorough next time 😄 Pushed a fix to the branch: 1a2329e. All stress tests for |
This was referenced Jun 14, 2024
This was referenced Jun 18, 2024
Build failure: Static graph-based restore failed with exit code .* but did not log an error.
#103526
Open
src/coreclr/jit/lsraarm64.cpp
Outdated
Comment on lines
2000
to
2004
| if (intrin.op2->OperIs(GT_HWINTRINSIC) && | ||
| (intrin.op2->AsHWIntrinsic()->GetHWIntrinsicId() == NI_Sve_ConvertVectorToMask)) | ||
| { | ||
| // Have RBM_ALLMASK candidates only if op2 is VectorToMask | ||
| assert(intrin.op2->gtType == TYP_MASK); |
There was a problem hiding this comment.
What's the reason for this? It's not clear why TYP_MASK would be restricted but VectorToMask would not...
There was a problem hiding this comment.
Without this, we end up assigning RBM_ALLMASK candidates for operands of AbsoluateCompare(), but they should be assigned RBM_ALLFLOAT candidates.
Comment on lines
1305
to
+1306
| // Wrap the intrinsic in ConditionalSelect only if it is not already inside another ConditionalSelect | ||
| if (!user->OperIsHWIntrinsic() || (user->AsHWIntrinsic()->GetHWIntrinsicId() != NI_Sve_ConditionalSelect)) | ||
| // If it is inside ConditionalSelect, then make sure that it is the `mask` operation of it. |
There was a problem hiding this comment.
Not sure I follow this entirely...
This is stating we need to wrap in a CndSel if the user isn't a CndSel or if it is but we're not the mask operand.
The !user->OperIsHWIntrinsic(NI_Sve_ConditionalSelect) seems fine with that regard but the HWIntrinsicInfo::ReturnsPerElementMask(node->GetHWIntrinsicId()) doesn't seem to fit.
Most notably we can have mask = CndSel(mask1, mask2, mask3) or vector CndSel(mask, vector1, vector2), but also I would have expected that intrinsics like Abs(x) which must be emitted with a mask to need CndSel regardless of where it appears and only that we could optimize it away in some cases like vector = CndSel(mask, vector1, CndSel(mask, vector2, vector3)) (where the outer and inner cndsel use identical masks that mean the latter CndSel becomes redundant as the computed operands would never be selected anyways).
There was a problem hiding this comment.
So I would have expected the latter condition to basically be conditioned differently, basically ensuring that intrinsics that require a CndSel wrapper always, optionally trying to fold in the case the mask usages can be detected as redundant
There was a problem hiding this comment.
It's better to rewrite the comment to avoid confusion I guess:
// Wrap the intrinsic in ConditionalSelect only if it is not already inside another ConditionalSelect
// or if it is but it is the `mask` operand.The goal of this block of code is to wrap HWIntrinsics which satisfy HWIntrinsicInfo::IsEmbeddedMaskedOperation(intrinsicId)) into ConditionalSelect. Usually this only needs to be done when the intrinsic is not already wrapped. But this logic doesn't hold when such intrinsic is used as the mask operand for ConditionalSelect - user->AsHWIntrinsic()->GetHWIntrinsicId() != NI_Sve_ConditionalSelect doesn't hold true any more, but the intrinsic still has to be wrapped ConditionalSelect (all that satisfy HWIntrinsicInfo::IsEmbeddedMaskedOperation(intrinsicId) have to).
This was referenced Jun 19, 2024
|
Replaced by #104464 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tests results: