Arm64/SVE: Implemented `ConvertToInt32` and `ConvertToUInt32` for float #103098

ebepho · 2024-06-05T22:26:12Z

Contributes to #99957

Stress test output:

===================Running default===================
------------------- {} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToInt32_int_float() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToUInt32_uint_float() : 7
===================Running jitstress===================
------------------- {'JitMinOpts': '1'} -------------------
------------------- {'JitStress': '1'} -------------------
------------------- {'JitStress': '2'} -------------------
------------------- {'JitStress': '1', 'TieredCompilation': '1'} -------------------
------------------- {'JitStress': '2', 'TieredCompilation': '1'} -------------------
------------------- {'TailcallStress': '1'} -------------------
------------------- {'ReadyToRun': '0'} -------------------
===================Running jitstressregs===================
------------------- {'JitStressRegs': '1'} -------------------
------------------- {'JitStressRegs': '2'} -------------------
------------------- {'JitStressRegs': '3'} -------------------
------------------- {'JitStressRegs': '4'} -------------------
------------------- {'JitStressRegs': '8'} -------------------
------------------- {'JitStressRegs': '0x10'} -------------------
------------------- {'JitStressRegs': '0x80'} -------------------
------------------- {'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStressRegs': '0x2000'} -------------------
===================Running jitstress2-jitstressregs===================
------------------- {'JitStress': '2', 'JitStressRegs': '1'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '2'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '3'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '4'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '8'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x10'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x80'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x2000'} -------------------

cc @dotnet/arm64-contrib

dotnet-issue-labeler · 2024-06-05T22:26:17Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2024-06-05T22:26:18Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-policy-service · 2024-06-05T22:26:39Z

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

kunalspathak

Added some comments.

kunalspathak · 2024-06-05T23:24:33Z

src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs

@@ -4203,7 +4203,7 @@ public new abstract partial class Arm64 : System.Runtime.Intrinsics.Arm.AdvSimd.
        public static System.Numerics.Vector<float> AbsoluteDifference(System.Numerics.Vector<float> left, System.Numerics.Vector<float> right) { throw null; }
        public static System.Numerics.Vector<ushort> AbsoluteDifference(System.Numerics.Vector<ushort> left, System.Numerics.Vector<ushort> right) { throw null; }
        public static System.Numerics.Vector<uint> AbsoluteDifference(System.Numerics.Vector<uint> left, System.Numerics.Vector<uint> right) { throw null; }
-        public static System.Numerics.Vector<ulong> AbsoluteDifference(System.Numerics.Vector<ulong> left, System.Numerics.Vector<ulong> right) { throw null; }        
+        public static System.Numerics.Vector<ulong> AbsoluteDifference(System.Numerics.Vector<ulong> left, System.Numerics.Vector<ulong> right) { throw null; }


can you undo the space change in this file?

kunalspathak · 2024-06-05T23:31:14Z

src/coreclr/jit/hwintrinsiclistarm64sve.h

@@ -32,6 +32,8 @@ HARDWARE_INTRINSIC(Sve,           Compute16BitAddresses,
 HARDWARE_INTRINSIC(Sve,           Compute32BitAddresses,                                            -1,      2,      true,  {INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_sve_adr,        INS_invalid,        INS_sve_adr,        INS_invalid,        INS_invalid},     HW_Category_SIMD,                  HW_Flag_Scalable|HW_Flag_SpecialCodeGen)
 HARDWARE_INTRINSIC(Sve,           Compute64BitAddresses,                                            -1,      2,      true,  {INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_sve_adr,        INS_invalid,        INS_sve_adr,        INS_invalid,        INS_invalid},     HW_Category_SIMD,                  HW_Flag_Scalable|HW_Flag_SpecialCodeGen)
 HARDWARE_INTRINSIC(Sve,           ConditionalSelect,                                                -1,      3,      true,  {INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel,        INS_sve_sel},     HW_Category_SIMD,                  HW_Flag_Scalable|HW_Flag_ExplicitMaskedOperation|HW_Flag_SupportsContainment)
+HARDWARE_INTRINSIC(Sve,           ConvertToInt32,                                                   -1,      -1,     false, {INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_sve_fcvtzs,     INS_invalid},     HW_Category_SIMD,                  HW_Flag_Scalable|HW_Flag_BaseTypeFromFirstArg|HW_Flag_SpecialCodeGen|HW_Flag_EmbeddedMaskedOperation|HW_Flag_LowMaskedOperation)


don't need HW_Flag_SpecialCodeGen flag

kunalspathak · 2024-06-05T23:32:48Z

src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs

@@ -3072,6 +3072,10 @@
    ("SveConditionalSelect.template",     new Dictionary<string, string> { ["TestName"] = "Sve_ConditionalSelect_uint",                                                                            ["Isa"] = "Sve",           ["LoadIsa"] = "Sve",     ["Method"] = "ConditionalSelect",                                                    ["RetVectorType"] = "Vector",    ["RetBaseType"] = "UInt32",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "UInt32", ["Op2VectorType"] = "Vector",    ["Op2BaseType"] = "UInt32", ["Op3VectorType"] = "Vector",    ["Op3BaseType"] = "UInt32", ["LargestVectorSize"] = "64",  ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()",       ["NextValueOp2"] = "TestLibrary.Generator.GetUInt32()",        ["NextValueOp3"] = "TestLibrary.Generator.GetUInt32()",                         ["ValidateIterResult"] = "(firstOp[i] != 0 ? (result[i] != secondOp[i]) : (result[i] != thirdOp[i]))",}),
    ("SveConditionalSelect.template",     new Dictionary<string, string> { ["TestName"] = "Sve_ConditionalSelect_ulong",                                                                           ["Isa"] = "Sve",           ["LoadIsa"] = "Sve",     ["Method"] = "ConditionalSelect",                                                    ["RetVectorType"] = "Vector",    ["RetBaseType"] = "UInt64",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "UInt64", ["Op2VectorType"] = "Vector",    ["Op2BaseType"] = "UInt64", ["Op3VectorType"] = "Vector",    ["Op3BaseType"] = "UInt64", ["LargestVectorSize"] = "64",  ["NextValueOp1"] = "TestLibrary.Generator.GetUInt64()",       ["NextValueOp2"] = "TestLibrary.Generator.GetUInt64()",        ["NextValueOp3"] = "TestLibrary.Generator.GetUInt64()",                         ["ValidateIterResult"] = "(firstOp[i] != 0 ? (result[i] != secondOp[i]) : (result[i] != thirdOp[i]))",}),

+    ("SveMasklessSimpleVecOpTest.template",        new Dictionary<string, string> { ["TestName"] = "Sve_ConvertToInt32_int_float",                                                                          ["Isa"] = "Sve",           ["LoadIsa"] = "Sve",     ["Method"] = "ConvertToInt32",                                                       ["RetVectorType"] = "Vector",    ["RetBaseType"] = "Int32",   ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "Single",                                                                                                                           ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()",                                                                                                                                                      ["ValidateIterResult"] = "Helpers.ConvertToInt32(firstOp[i]) != result[i]", ["GetIterResult"] = "Helpers.ConvertToInt32(leftOp[i])"}),


we should use SveSimpleVecOpTest.template here.

We ended up needing a new test template to handle the fact that these APIs' return types are different from their input types, so the ConditionalSelect scenarios' first and third operands need to match the API's return type. The new template is otherwise functionally equivalent to SveSimpleVecOpTest.template, though we removed the ConditionalSelect_Op1 scenario since ConditionalSelectScenario(_mask, _fld1, _fld1); doesn't satisfy the new method signature.

Removing ConditionalSelect_Op1 () makes sense, but apart from that, do you think we could have fixed SveSimpleVecOpTest.template to have RetType wherever necessary and then just update the value of it to be same as Op1BaseType for other tests and having a different one for Convert API?

That makes sense to me, though would you be ok with us getting rid of ConditionalSelect_Op1 for all existing tests using SveSimpleVecOpTest.template? If we aren't losing anything valuable from that, then I can merge the two templates in a follow-up PR after this one is merged.

We definitely need ConditionalSelect_Op1, but was wondering if other than it can be fixed or not. We can probably merge under the condition of if (typeof({Op1BaseType}) == typeof({RetBaseType}) or something.

I see, I can add that check

if (typeof({Op1BaseType}) == typeof({RetBaseType}) - I'm not sure if this would work at the compile of the .cs file. For the mismatching cases, the code inside the if block would have mismatching types, and probably get type mismatch errors. Would work if Rosyln just optimises it all away. Alternatively, you can probably add a lot of casts.

Reducing the number of templates is good.

While your cleaning this up, the conditional tests it lots of places use {Op1VectorType}<{RetBaseType>. These should ideally be {RetVectorType}<{RetBaseType> or {Op1VectorType}<{Op1BaseType>. Doesn't matter which as they'll be the same.

updating branch due to assert issue.

kunalspathak

LGTM. Thanks!

amanasifkhalid · 2024-06-11T22:11:33Z

@ebepho ready to merge? Once Build Analysis is finished running and the "squash and merge" button isn't greyed out, feel free to add a short commit message and merge.

kunalspathak · 2024-06-12T00:17:55Z

/ba-g failures seems to be timeout

ebepho added 2 commits June 5, 2024 14:30

Added ConverToInt32 and ConvertToUInt32 for float inputs.

c738b77

Added flags to handle only low predicate registers.

35d39d9

dotnet-issue-labeler bot added area-System.Runtime.Intrinsics new-api-needs-documentation labels Jun 5, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2024

amanasifkhalid added the arm-sve Work related to arm64 SVE/SVE2 support label Jun 5, 2024

ebepho removed the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2024

Fixed merge conflicts.

7a781e1

kunalspathak requested changes Jun 5, 2024

View reviewed changes

Fix whitespace

1378d60

This was referenced Jun 6, 2024

Wasm test failure: System addon update list error SyntaxError: XMLHttpRequest.open #102955

Closed

[wasm] ystem addon update list error SyntaxError: XMLHttpRequest.open: 'http://%(server)s/dummy-system-addons.xml' is not a valid URL. #103060

Closed

ebepho added 4 commits June 7, 2024 14:22

Remove special codegen flag

10c7a15

Added new test template for operations with different return types.

8004868

Merge branch 'main' into ConvertToInt32

af7ccd4

updating branch due to assert issue.

Add new test template.

8cb76da

amanasifkhalid requested a review from kunalspathak June 11, 2024 19:01

kunalspathak approved these changes Jun 11, 2024

View reviewed changes

kunalspathak merged commit ced2117 into dotnet:main Jun 12, 2024
154 of 167 checks passed

kunalspathak mentioned this pull request Jun 12, 2024

Arm64: Implement SVE APIs #99957

Closed

ebepho changed the title ~~Arm64/SVE: Implement ConvertToInt32 and ConvertToUInt32 for float~~ Arm64/SVE: Implemented ConvertToInt32 and ConvertToUInt32 for float Jun 17, 2024

github-actions bot locked and limited conversation to collaborators Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arm64/SVE: Implemented `ConvertToInt32` and `ConvertToUInt32` for float #103098

Arm64/SVE: Implemented `ConvertToInt32` and `ConvertToUInt32` for float #103098

ebepho commented Jun 5, 2024

dotnet-issue-labeler bot commented Jun 5, 2024

dotnet-issue-labeler bot commented Jun 5, 2024

dotnet-policy-service bot commented Jun 5, 2024

kunalspathak left a comment

kunalspathak Jun 5, 2024

kunalspathak Jun 5, 2024

kunalspathak Jun 5, 2024

amanasifkhalid Jun 11, 2024

kunalspathak Jun 11, 2024

amanasifkhalid Jun 11, 2024

kunalspathak Jun 11, 2024

amanasifkhalid Jun 11, 2024

a74nh Jun 12, 2024

kunalspathak left a comment

amanasifkhalid commented Jun 11, 2024 •

edited

Loading

kunalspathak commented Jun 12, 2024

Arm64/SVE: Implemented ConvertToInt32 and ConvertToUInt32 for float #103098

Arm64/SVE: Implemented ConvertToInt32 and ConvertToUInt32 for float #103098

Conversation

ebepho commented Jun 5, 2024

dotnet-issue-labeler bot commented Jun 5, 2024

dotnet-issue-labeler bot commented Jun 5, 2024

dotnet-policy-service bot commented Jun 5, 2024

kunalspathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak left a comment

Choose a reason for hiding this comment

amanasifkhalid commented Jun 11, 2024 • edited Loading

kunalspathak commented Jun 12, 2024

Arm64/SVE: Implemented `ConvertToInt32` and `ConvertToUInt32` for float #103098

Arm64/SVE: Implemented `ConvertToInt32` and `ConvertToUInt32` for float #103098

amanasifkhalid commented Jun 11, 2024 •

edited

Loading