JIT ARM64-SVE: Add Sve.ConditionalExtract* APIs #104150
Merged
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
|
Note regarding the |
1 similar comment
|
Note regarding the |
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
|
@kunalspathak @dotnet/arm64-contrib @a74nh |
This was referenced Jun 28, 2024
Open
|
@TIHan - can you please take a preliminary look? |
Did you already synced with |
I'm not sure, could you specify the commit in question so I can check? |
#104065. Please sync with |
mikabl-arm
force-pushed
the
ConditionalExtract-github
branch
from
5400b45
to
97c9682
Compare
|
Thank you, @kunalspathak , I can confirm that the tests pass successfully now: |
kunalspathak
requested changes
Jul 2, 2024
|
|
||
| /// <summary> | ||
| /// svint16_t svclasta[_s16](svbool_t pg, svint16_t fallback, svint16_t data) | ||
| /// CLASTA Ztied.H, Pg, Ztied.H, Zdata.H |
There was a problem hiding this comment.
This instruction maps to the API that takes Vector<T> for defaultValue parameter. Please remove it from here and at other places.
...braries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Sve.PlatformNotSupported.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Sve.cs
Outdated
Show resolved
Hide resolved
| { | ||
| opt = emitter::optGetSveInsOpt(emitTypeSize(node->GetSimdBaseType())); | ||
|
|
||
| if (emitter::isGeneralRegisterOrZR(targetReg)) |
There was a problem hiding this comment.
shouldn't this be an assert instead? assert(emitter::isGeneralRegisterOrZR(targetReg));?
There was a problem hiding this comment.
Or are we using the term *Scalar for instructions that are generated for https://docsmirror.github.io/A64/2023-06/clasta_r_p_z.html and https://docsmirror.github.io/A64/2023-06/clasta_v_p_z.html?
There was a problem hiding this comment.
CLASTA (scalar) is emitted only for integer *Scalar APIs. For floating-point *Scalar APIs we have to emit CLASTA (SIMD&FP scalar) instead.
| @@ -3914,6 +3917,67 @@ | |||
| ("SveVecReduceUnOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ZeroExtendWideningUpper_uint_ushort", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ZeroExtendWideningUpper", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt16", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt16()", ["ValidateReduceOpResult"] = "Helpers.ZeroExtendWideningUpper(firstOp, 0) != result[0]", ["ValidateRemainingResults"] = "Helpers.ZeroExtendWideningUpper(firstOp, i) != result[i]"}), | |||
| ("SveVecReduceUnOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ZeroExtendWideningUpper_ulong_uint", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ZeroExtendWideningUpper", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt64", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt32", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()", ["ValidateReduceOpResult"] = "Helpers.ZeroExtendWideningUpper(firstOp, 0) != result[0]", ["ValidateRemainingResults"] = "Helpers.ZeroExtendWideningUpper(firstOp, i) != result[i]"}), | |||
|
|
|||
| ("SveVecTernOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ConditionalExtractAfterLastActiveElement_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConditionalExtractAfterLastActiveElement", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["Op3VectorType"] = "Vector", ["Op3BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp3"] = "TestLibrary.Generator.GetSingle()", ["ValidateIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(firstOp, secondOp, thirdOp)[i] != result[i]", ["GetIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(first, second, third)[i]", ["ConvertFunc"] = ""}), | |||
There was a problem hiding this comment.
Please place these in alphabetical order.
| @@ -3914,6 +3917,67 @@ | |||
| ("SveVecReduceUnOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ZeroExtendWideningUpper_uint_ushort", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ZeroExtendWideningUpper", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt16", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt16()", ["ValidateReduceOpResult"] = "Helpers.ZeroExtendWideningUpper(firstOp, 0) != result[0]", ["ValidateRemainingResults"] = "Helpers.ZeroExtendWideningUpper(firstOp, i) != result[i]"}), | |||
| ("SveVecReduceUnOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ZeroExtendWideningUpper_ulong_uint", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ZeroExtendWideningUpper", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt64", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt32", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()", ["ValidateReduceOpResult"] = "Helpers.ZeroExtendWideningUpper(firstOp, 0) != result[0]", ["ValidateRemainingResults"] = "Helpers.ZeroExtendWideningUpper(firstOp, i) != result[i]"}), | |||
|
|
|||
| ("SveVecTernOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ConditionalExtractAfterLastActiveElement_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConditionalExtractAfterLastActiveElement", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["Op3VectorType"] = "Vector", ["Op3BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp3"] = "TestLibrary.Generator.GetSingle()", ["ValidateIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(firstOp, secondOp, thirdOp)[i] != result[i]", ["GetIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(first, second, third)[i]", ["ConvertFunc"] = ""}), | |||
| ("SveScalarTernOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ConditionalExtractAfterLastActiveElement_float_scalar", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConditionalExtractAfterLastActiveElement", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2BaseType"] = "Single", ["Op3VectorType"] = "Vector", ["Op3BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp3"] = "TestLibrary.Generator.GetSingle()", ["ValidateScalarResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(firstOp, secondOp, thirdOp) != result"}), | |||
| ("SveVecTernOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ConditionalExtractAfterLastActiveElement_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConditionalExtractAfterLastActiveElement", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Double", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Double", ["Op3VectorType"] = "Vector", ["Op3BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetDouble()", ["NextValueOp2"] = "TestLibrary.Generator.GetDouble()", ["NextValueOp3"] = "TestLibrary.Generator.GetDouble()", ["ValidateIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(firstOp, secondOp, thirdOp)[i] != result[i]", ["GetIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(first, second, third)[i]", ["ConvertFunc"] = ""}), | |||
There was a problem hiding this comment.
also, it will be good to group the same APIs under test together.
|
Looks like I missed several issues while testing the updated changes locally. Please wait 'till they are resolved before re-reviewing. |
Ah just seeing this message. Will reset the approval status. |
mikabl-arm
force-pushed
the
ConditionalExtract-github
branch
from
0b82f53
to
b0212ca
Compare
|
Updated, the stress testing passes. |
kunalspathak
requested changes
Jul 5, 2024
src/coreclr/jit/gentree.cpp
Outdated
| NI_Sve_GatherVectorUInt16WithByteOffsetsZeroExtend, NI_Sve_GatherVectorUInt16ZeroExtend, | ||
| NI_Sve_GatherVectorUInt32WithByteOffsetsZeroExtend, NI_Sve_GatherVectorUInt32ZeroExtend)); | ||
| assert(varTypeIsI(addr) || (varTypeIsSIMD(addr) && ((intrinsicId >= NI_Sve_GatherVector) && | ||
| AreContiguous(NI_Sve_GatherPrefetch16Bit, NI_Sve_GatherPrefetch32Bit, NI_Sve_GatherPrefetch64Bit, |
There was a problem hiding this comment.
This is not needed after #104396. Can you please merge the main and revert this?
| @@ -4087,6 +4090,67 @@ | |||
| ("SveVecReduceUnOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ZeroExtendWideningUpper_uint_ushort", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ZeroExtendWideningUpper", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt16", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt16()", ["ValidateReduceOpResult"] = "Helpers.ZeroExtendWideningUpper(firstOp, 0) != result[0]", ["ValidateRemainingResults"] = "Helpers.ZeroExtendWideningUpper(firstOp, i) != result[i]"}), | |||
| ("SveVecReduceUnOpTest.template",new Dictionary<string, string> {["TestName"] = "Sve_ZeroExtendWideningUpper_ulong_uint", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ZeroExtendWideningUpper", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt64", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt32", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()", ["ValidateReduceOpResult"] = "Helpers.ZeroExtendWideningUpper(firstOp, 0) != result[0]", ["ValidateRemainingResults"] = "Helpers.ZeroExtendWideningUpper(firstOp, i) != result[i]"}), | |||
|
|
|||
| ("SveVecTernOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_ConditionalExtractAfterLastActiveElement_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConditionalExtractAfterLastActiveElement", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["Op3VectorType"] = "Vector", ["Op3BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp3"] = "TestLibrary.Generator.GetSingle()", ["ValidateIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(firstOp, secondOp, thirdOp)[i] != result[i]",["GetIterResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(first, second, third)[i]", ["ConvertFunc"] = " ",}), | |||
| ("SveScalarTernOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_ConditionalExtractAfterLastActiveElement_float_scalar", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConditionalExtractAfterLastActiveElement", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2BaseType"] = "Single", ["Op3VectorType"] = "Vector", ["Op3BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp3"] = "TestLibrary.Generator.GetSingle()", ["ValidateScalarResult"] = "Helpers.ConditionalExtractAfterLastActiveElement(firstOp, secondOp, thirdOp) != result",}), | |||
There was a problem hiding this comment.
please align the lines for the newly added tests.
This was referenced Jul 5, 2024
Build failure: Static graph-based restore failed with exit code .* but did not log an error.
#103526
Open
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.
Testing
Regular tests pass successfully. Stress testing highlights two issues. The first one as I believe is already know and decided to be fixed later. It results in a method returning a value as if a mask/predicate register were all false despite that error output show that it's not, e.g.
The second one manifests in a form of a method returning a zeroed vector. It generally looks like the following and appears for all kinds of types and ConditinalExtract* APIs but only for RunBasicScenario_Load tests:
A one would expect the
resultto be(92, ... , 92)here or(83, 22, 34, 122, 111, 34, 25, 103, 44, 98, 37, 4, 13, 85, 93, 12)(same assecondOp) if the first issue discussed above manifested here. But all zeros are something different. This behavior can be attributed to the first issue if we consider how it can interfere withRunBasicScenario_Load: ifloadMask3is all false (incorrectly as the effect of the first issue), then both the third arg and the result ofConditionalExtractAfterLastActiveElementAndReplicatewould be all zeroes. The same would happen in case allloadMask*s are all false. We would see different results if only eitherloadMask1orloadMask2is all false, but I can't see anything like that in the logs.Test logs
ConditionalExtract-stress.txt