Arm64/Sve: Implement LoadVector*NonFaultingSignExtendTo* APIs by kunalspathak · Pull Request #102903 · dotnet/runtime

kunalspathak · 2024-05-30T23:28:04Z

All stress tests passing: https://gist.github.com/kunalspathak/355fa828453c9f4fbe76c7d4dbfa0d9a

Contributes to #99957

ghost · 2024-05-30T23:28:10Z

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.

kunalspathak · 2024-05-30T23:28:17Z

@dotnet/arm64-contrib @TIHan PTAL

dotnet-policy-service · 2024-05-30T23:28:32Z

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

src/coreclr/jit/hwintrinsiclistarm64sve.h

TIHan

LGTM. Minor comment on the category. Test template works great.

a74nh · 2024-05-31T07:39:24Z

src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs

-    ("SveLoadMaskedUnOpTest.template",    new Dictionary<string, string> { ["TestName"] = "SveLoadVectorUInt16ZeroExtendToUInt64",                                                                 ["Isa"] = "Sve",                                    ["Method"] = "LoadVectorUInt16ZeroExtendToUInt64",                                   ["RetVectorType"] = "Vector",    ["RetBaseType"] = "UInt64",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "UInt64",                                  ["Op2BaseType"] = "UInt16",                                                              ["LargestVectorSize"] = "64",                                                                ["NextValueOp2"] = "TestLibrary.Generator.GetUInt16()",                                                                                        ["ValidateIterResult"] = "firstOp[i] != result[i]"}),
-    ("SveLoadMaskedUnOpTest.template",    new Dictionary<string, string> { ["TestName"] = "SveLoadVectorUInt32ZeroExtendToInt64",                                                                  ["Isa"] = "Sve",                                    ["Method"] = "LoadVectorUInt32ZeroExtendToInt64",                                    ["RetVectorType"] = "Vector",    ["RetBaseType"] = "Int64",   ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "Int64",                                   ["Op2BaseType"] = "UInt32",                                                              ["LargestVectorSize"] = "64",                                                                ["NextValueOp2"] = "TestLibrary.Generator.GetUInt32()",                                                                                        ["ValidateIterResult"] = "firstOp[i] != result[i]"}),
-    ("SveLoadMaskedUnOpTest.template",    new Dictionary<string, string> { ["TestName"] = "SveLoadVectorUInt32ZeroExtendToUInt64",                                                                 ["Isa"] = "Sve",                                    ["Method"] = "LoadVectorUInt32ZeroExtendToUInt64",                                   ["RetVectorType"] = "Vector",    ["RetBaseType"] = "UInt64",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "UInt64",                                  ["Op2BaseType"] = "UInt32",                                                              ["LargestVectorSize"] = "64",                                                                ["NextValueOp2"] = "TestLibrary.Generator.GetUInt32()",                                                                                        ["ValidateIterResult"] = "firstOp[i] != result[i]"}),
+    ("SveLoadMaskedUnOpTest.template",    new Dictionary<string, string> { ["TestName"] = "Sve_LoadVector_float",                                                                                   ["Isa"] = "Sve",                                    ["Method"] = "LoadVector",                                                           ["RetVectorType"] = "Vector",    ["RetBaseType"] = "Single",  ["Op1VectorType"] = "Vector",    ["Op1BaseType"] = "Single",                                  ["Op2BaseType"] = "Single",                                                              ["LargestVectorSize"] = "64",                                                                ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()",                                                                                        ["ValidateIterResult"] = "firstOp[i] != result[i]"}),


What did you change on these lines? Was it just the TestName?

yes, just to have them consistent naming of Sve_*

a74nh · 2024-05-31T07:41:25Z

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

+                // Validates passing an instance member of a struct works
+                test.RunStructFldScenario();
+
+                // Validates using inside ConditionalSelect with value falseValue


I'm surprised we've not hit this before - there are other instructions that are Pg/Z.

All the APIs we have implemented that has Pg/Z are either the ones that explicitly takes mask as first argument like LoadVector for which we don't use it inside ConditionalSelect or the ones that operates on purely predicate registers like EOR Presult.B, Pg/Z, Pop1.B, Pop2.B which we have not exposed it via API. This is the first API that implicitly has a mask argument and has Pg/Z format in the instruction.

a74nh · 2024-05-31T07:44:15Z

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

+
+                // Validates using inside ConditionalSelect with zero falseValue
+                test.ConditionalSelect_ZeroOp();
+            }


Maybe not for this PR, but I remember discussion about testing with addresses that fault. Use an address that is invalid/partially invalid, then test an exception is not raised.
Then maybe add the same test to the normal loads and check it does fault.

Yes, @TIHan added that in #102180

runtime/src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveLoad2xVectorAndUnzipTest.template

Lines 190 to 207 in 3750ac5

//TODO-SVE: Once register allocation exists for predicates, move loadMask into DataTable

{Op1VectorType}<{Op1BaseType}> loadTrueMask = Sve.CreateTrueMask{RetBaseType}(SveMaskPattern.All);

{Op1VectorType}<{Op1BaseType}> loadFalseMask = Sve.CreateFalseMask{RetBaseType}();

var result = {Isa}.{Method}(loadTrueMask, ({Op1BaseType}*)(_dataTable.inArrayPtr));

try

{

result = {Isa}.{Method}(loadFalseMask, default);

Unsafe.Write(_dataTable.outArray1Ptr, result.Item1);

Unsafe.Write(_dataTable.outArray2Ptr, result.Item2);

ValidateZeroResult(_dataTable.outArray1Ptr, _dataTable.outArray2Ptr, _dataTable.inArrayPtr);

}

catch

{

Succeeded = false;

}

a74nh · 2024-05-31T07:44:48Z

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

@@ -0,0 +1,413 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.


What's special about this template that means it can't use the standard load template?

Having a RunBasicScenario_LoadNonFaulting() test case that will make sure we do not fault even for invalid memory.

I was looking for a try/catch ... but we don't need to add that.

kunalspathak · 2024-05-31T13:09:09Z

/ba-g opened #102919

kunalspathak added 9 commits May 30, 2024 06:21

Add *NonFaultingSignExtendTo* APIs

6609e51

Map APIs to instructions

17c0f2b

add test coverage

863c6e7

Rename some of the test names

8a3b4e8

fix test cases

f2a11fd

fix the codegen

47e8016

ignore gpr

926e324

fix test template

5719249

Comment ConditionalSelect_FalseOp()

5de2c29

kunalspathak requested a review from TIHan May 30, 2024 23:28

ghost added area-System.Runtime.Intrinsics new-api-needs-documentation labels May 30, 2024

dotnet-policy-service bot assigned kunalspathak May 30, 2024

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

TIHan reviewed May 31, 2024

View reviewed changes

src/coreclr/jit/hwintrinsiclistarm64sve.h Outdated Show resolved Hide resolved

TIHan approved these changes May 31, 2024

View reviewed changes

build-analysis bot mentioned this pull request May 31, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Closed

review feedback

acbd50c

a74nh reviewed May 31, 2024

View reviewed changes

kunalspathak mentioned this pull request May 31, 2024

Source generator tests failing with AV in Roslyn #102919

Closed

kunalspathak merged commit 214cbbf into dotnet:main May 31, 2024

kunalspathak deleted the loadvector-1 branch May 31, 2024 13:09

kunalspathak mentioned this pull request May 31, 2024

Arm64: Implement SVE APIs #99957

Closed

kunalspathak mentioned this pull request Jun 7, 2024

ARM64-SVE: gathervector #103159

Merged

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

	//TODO-SVE: Once register allocation exists for predicates, move loadMask into DataTable
	{Op1VectorType}<{Op1BaseType}> loadTrueMask = Sve.CreateTrueMask{RetBaseType}(SveMaskPattern.All);
	{Op1VectorType}<{Op1BaseType}> loadFalseMask = Sve.CreateFalseMask{RetBaseType}();

	var result = {Isa}.{Method}(loadTrueMask, ({Op1BaseType}*)(_dataTable.inArrayPtr));

	try
	{
	result = {Isa}.{Method}(loadFalseMask, default);

	Unsafe.Write(_dataTable.outArray1Ptr, result.Item1);
	Unsafe.Write(_dataTable.outArray2Ptr, result.Item2);
	ValidateZeroResult(_dataTable.outArray1Ptr, _dataTable.outArray2Ptr, _dataTable.inArrayPtr);
	}
	catch
	{
	Succeeded = false;
	}

		@@ -0,0 +1,413 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// The .NET Foundation licenses this file to you under the MIT license.

Conversation

kunalspathak commented May 30, 2024

Uh oh!

ghost commented May 30, 2024

Uh oh!

kunalspathak commented May 30, 2024

Uh oh!

dotnet-policy-service bot commented May 30, 2024

Uh oh!

Uh oh!

TIHan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented May 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments