ARM64-SVE: Implement `ShiftLeftLogical`, `ShiftRightArithmetic`, `ShiftRightLogical` #104119

amanasifkhalid · 2024-06-27T19:44:10Z

Part of #99957. As discussed offline with @kunalspathak, some of the newly-added tests are failing under JitStress scenarios due to known callee-save register issues. Other than those cases, all tests are passing under the other stress modes -- the output is large, but I'm happy to share it if it's of interest.

These new APIs are unique in that their operands can differ in vector widths. To avoid introducing a new test template, I tweaked the existing vector binop test template to parse the second operand's type correctly, and added some preprocessor logic to conditionally compile certain tests that require the operand types to be the same. It looks a bit hacky, but it doesn't affect existing tests that have matching operand types.

@dotnet/arm64-contrib PTAL, thanks!

dotnet-issue-labeler · 2024-06-27T19:44:15Z

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-27T19:44:16Z

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-27T19:44:38Z

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

amanasifkhalid · 2024-06-27T19:51:48Z

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

@@ -558,6 +558,28 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
                {
                    assert(instrIsRMW);

+                    insScalableOpts sopt;
+
+                    switch (intrinEmbMask.id)


I'm not sure if this is the best place to put this switch statement, but these intrinsics need to be wrapped with ConditionalSelect, so implementing special codegen for them off this path doesn't seem feasible without some more invasive refactoring.

Yes, we do similar handling for ConvertTo for numOperands == 1 case.

kunalspathak

Overall looks good. Added some questions around testing.

kunalspathak · 2024-06-28T00:47:31Z

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Sve.cs

+        ///   LSL Ztied1.S, Pg/M, Ztied1.S, Zop2.S
+        /// svuint32_t svlsl[_u32]_x(svbool_t pg, svuint32_t op1, svuint32_t op2)
+        ///   LSL Ztied1.S, Pg/M, Ztied1.S, Zop2.S
+        ///   LSLR Ztied2.S, Pg/M, Ztied2.S, Zop1.S


I don't think we are doing LSLR. Can you please remove it from summary docs from this and file?

kunalspathak · 2024-06-28T00:47:38Z

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Sve.cs

+        ///   ASR Ztied1.H, Pg/M, Ztied1.H, Zop2.H
+        /// svint16_t svasr[_s16]_x(svbool_t pg, svint16_t op1, svuint16_t op2)
+        ///   ASR Ztied1.H, Pg/M, Ztied1.H, Zop2.H
+        ///   ASRR Ztied2.H, Pg/M, Ztied2.H, Zop1.H


there are actually bunch of places where we have also added "XXXR". please remove it.

kunalspathak · 2024-06-28T00:51:10Z

src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs

+    ("SveVecBinOpTest.template",      new Dictionary<string, string> { ["TestName"] = "Sve_ShiftLeftLogical_ushort_ulong",                                                                         ["Isa"] = "Sve",           ["LoadIsa"] = "Sve",     ["Method"] = "ShiftLeftLogical",                                                     ["RetVectorType"] = "Vector",    ["RetBaseType"] = "UInt16",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "UInt16", ["Op2VectorType"] = "Vector",    ["Op2BaseType"] = "UInt64",                                                                       ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt16()",        ["NextValueOp2"] = "TestLibrary.Generator.GetUInt64()",                                                      ["ValidateIterResult"] = "Helpers.ShiftLeft<ushort>(left[i], right[(i * sizeof(ushort)) / sizeof(ulong)]) != result[i]",                ["GetIterResult"] = "Helpers.ShiftLeft<ushort>(left[i], right[(i * sizeof(ushort)) / sizeof(ulong)])"}),
+    ("SveVecBinOpTest.template",      new Dictionary<string, string> { ["TestName"] = "Sve_ShiftLeftLogical_uint_ulong",                                                                           ["Isa"] = "Sve",           ["LoadIsa"] = "Sve",     ["Method"] = "ShiftLeftLogical",                                                     ["RetVectorType"] = "Vector",    ["RetBaseType"] = "UInt32",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "UInt32", ["Op2VectorType"] = "Vector",    ["Op2BaseType"] = "UInt64",                                                                       ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()",        ["NextValueOp2"] = "TestLibrary.Generator.GetUInt64()",                                                      ["ValidateIterResult"] = "Helpers.ShiftLeft<uint>(left[i], right[(i * sizeof(uint)) / sizeof(ulong)]) != result[i]",                    ["GetIterResult"] = "Helpers.ShiftLeft<uint>(left[i], right[(i * sizeof(uint)) / sizeof(ulong)])"}),
+
+    ("SveVecBinOpTest.template",      new Dictionary<string, string> { ["TestName"] = "Sve_ShiftRightArithmetic_sbyte_byte",                                                                       ["Isa"] = "Sve",           ["LoadIsa"] = "Sve",     ["Method"] = "ShiftRightArithmetic",                                                 ["RetVectorType"] = "Vector",    ["RetBaseType"] = "SByte",   ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "SByte",  ["Op2VectorType"] = "Vector",    ["Op2BaseType"] = "Byte",                                                                         ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSByte()",         ["NextValueOp2"] = "TestLibrary.Generator.GetByte()",                                                        ["ValidateIterResult"] = "Helpers.ShiftRight<sbyte>(left[i], (ulong)right[i]) != result[i]",                                            ["GetIterResult"] = "Helpers.ShiftRight<sbyte>(left[i], (ulong)right[i])"}),


I was hoping to see something like Helpers.ShiftRightArithmetic for validation for these APIs?

The generic ShiftRight helper will do either arithmetic or logical shift, depending on whether the first operand is signed or not since it's just doing division. I can write a ShiftRightArithmetic wrapper for ShiftRight that only takes signed types for the first operand to make the API surface clearer, though I was hoping to avoid having to write multiple overloads for sbyte, short, int, etc -- which would you prefer?

kunalspathak · 2024-06-28T00:57:04Z

src/tests/JIT/HardwareIntrinsics/Arm/Shared/Helpers.cs

@@ -1943,6 +1943,28 @@ private static int HighNarrowing(long op1, bool round)

        public static long MultiplyWideningUpperAndSubtract(long[] op1, int[] op2, int[] op3, int i) => MultiplyWideningAndSubtract(op1[i], op2[i + op2.Length / 2], op3[i + op3.Length / 2]);

+        public static T ShiftLeft<T>(T op1, ulong op2) where T : INumber<T>


can we use the existing Helpers.ShiftLeftLogical()?

I don't think those match the functionality of these APIs: The existing UnsignedShift seems to only consider the first byte of the shift value, regardless of the size of the input, whereas the SVE APIs consider the whole second operand (docs). I agree the code duplication isn't ideal -- I'm happy to rename the new helpers to something more discerning, if you'd like.

kunalspathak · 2024-06-28T00:59:38Z

src/tests/JIT/HardwareIntrinsics/Arm/Shared/_SveBinaryOpTestTemplate.template

+// If they aren't the same, the #undef won't do anything,
+// and the first operand type symbol will remain defined.
+// We can then conditionally compile on the condition #if !{Op1VectorType}{Op1BaseType}, etc.
+#define {Op1VectorType}{Op1BaseType}


I think the intent is right and I suggested something along these lines at one point, but looking at this, it feels little odd and might be hard to maintain, specially because we are not careful in setting Op1BaseType and Op2BaseType. Lets add a new template (if you did not find the one to reuse) and we will clean up them once we are done with .NET 9 implementation.

Got it; I copied my changes to _SveBinaryOpTestTemplate over to _SveBinaryOpDifferentTypesTestTemplate (minus the conditional compilation stuff), and reverted my changes to the former.

amanasifkhalid · 2024-06-28T14:41:46Z

@kunalspathak stress tests still pass (minus the known callee-save issues) with the template changes.

kunalspathak

LGTM

amanasifkhalid added 7 commits June 24, 2024 17:36

Wip

5923461

Merge from main

a230294

Working for same-operand types

00078c9

Add ShiftleftLogical, ShiftRightLogical

84f68f3

Style

53d8baf

Add ShiftRightArithmetic

6183309

Remove ShiftRightArithmeticForDivide

ff37d83

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

dotnet-policy-service bot assigned amanasifkhalid Jun 27, 2024

amanasifkhalid mentioned this pull request Jun 27, 2024

Arm64: Implement SVE APIs #99957

Closed

amanasifkhalid commented Jun 27, 2024

View reviewed changes

amanasifkhalid added 2 commits June 27, 2024 16:02

Add fallthrough annotation

382c8d3

Align test templates

af1461e

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

kunalspathak requested changes Jun 28, 2024

View reviewed changes

This was referenced Jun 28, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

The job running on agent NetCore-Public ran longer than the maximum time #104044

Closed

amanasifkhalid added 2 commits June 28, 2024 09:46

Update doc comments

843dcde

New test template

9080f4e

Remove unnecessary changes

9616ba3

kunalspathak approved these changes Jun 28, 2024

View reviewed changes

amanasifkhalid mentioned this pull request Jun 28, 2024

System.Security.Cryptography.Csp.Tests failing on macOS x64 Debug #104172

Open

amanasifkhalid closed this Jun 28, 2024

amanasifkhalid reopened this Jun 28, 2024

build-analysis bot mentioned this pull request Jun 28, 2024

Build failure: Static graph-based restore failed with exit code .* but did not log an error. #103526

Open

This was referenced Jun 28, 2024

System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows fails in CI #103586

Open

Build failure: Static graph-based restore failed with exit code .* but did not log an error. dotnet/dnceng#3139

Closed

amanasifkhalid merged commit 33fb7cd into dotnet:main Jun 28, 2024
159 of 167 checks passed

amanasifkhalid deleted the sve-shift branch June 28, 2024 19:20

kunalspathak mentioned this pull request Jun 29, 2024

Arm64/Sve: Test build failing because of ShiftLeftLogical/ShiftRightLogical #104190

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARM64-SVE: Implement `ShiftLeftLogical`, `ShiftRightArithmetic`, `ShiftRightLogical` #104119

ARM64-SVE: Implement `ShiftLeftLogical`, `ShiftRightArithmetic`, `ShiftRightLogical` #104119

amanasifkhalid commented Jun 27, 2024

dotnet-issue-labeler bot commented Jun 27, 2024

dotnet-issue-labeler bot commented Jun 27, 2024

dotnet-policy-service bot commented Jun 27, 2024

amanasifkhalid Jun 27, 2024

kunalspathak Jun 28, 2024

kunalspathak left a comment

kunalspathak Jun 28, 2024

kunalspathak Jun 28, 2024

kunalspathak Jun 28, 2024

kunalspathak Jun 28, 2024

amanasifkhalid Jun 28, 2024

kunalspathak Jun 28, 2024

amanasifkhalid Jun 28, 2024

kunalspathak Jun 28, 2024

amanasifkhalid Jun 28, 2024

amanasifkhalid commented Jun 28, 2024 •

edited

Loading

kunalspathak left a comment

		@@ -1943,6 +1943,28 @@ private static int HighNarrowing(long op1, bool round)

		public static long MultiplyWideningUpperAndSubtract(long[] op1, int[] op2, int[] op3, int i) => MultiplyWideningAndSubtract(op1[i], op2[i + op2.Length / 2], op3[i + op3.Length / 2]);

		public static T ShiftLeft<T>(T op1, ulong op2) where T : INumber<T>

ARM64-SVE: Implement ShiftLeftLogical, ShiftRightArithmetic, ShiftRightLogical #104119

ARM64-SVE: Implement ShiftLeftLogical, ShiftRightArithmetic, ShiftRightLogical #104119

Conversation

amanasifkhalid commented Jun 27, 2024

dotnet-issue-labeler bot commented Jun 27, 2024

dotnet-issue-labeler bot commented Jun 27, 2024

dotnet-policy-service bot commented Jun 27, 2024

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

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

amanasifkhalid commented Jun 28, 2024 • edited Loading

kunalspathak left a comment

Choose a reason for hiding this comment

ARM64-SVE: Implement `ShiftLeftLogical`, `ShiftRightArithmetic`, `ShiftRightLogical` #104119

ARM64-SVE: Implement `ShiftLeftLogical`, `ShiftRightArithmetic`, `ShiftRightLogical` #104119

amanasifkhalid commented Jun 28, 2024 •

edited

Loading