Add support for Sve.Store() #102262

SwapnilGaikwad · 2024-05-15T14:58:33Z

Contribute towards #99957.

dotnet-issue-labeler · 2024-05-15T14:58:39Z

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.

SwapnilGaikwad · 2024-05-15T14:59:05Z

@a74nh @kunalspathak @dotnet/arm64-contrib @arch-arm64-sve

dotnet-policy-service · 2024-05-15T14:59:09Z

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

kunalspathak

This is a big PR and thanks for working on it. Overall, looks good except some nit comments. Also, please run the tests, including the stress tests and share the results.

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

src/coreclr/jit/hwintrinsiclistarm64sve.h

a74nh · 2024-05-16T08:55:57Z

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

+ break;
+ }
+
+ case NI_Sve_Store:


Looking at #102180 reminded me that we can do the following....

For NI_Sve_Store remove the Special Code Gen flag and this case statement. This will cause the generic code to call emitIns_R_R_R().

You can now add a special case in emitIns_R_R_R() so that for NI_Sve_Store it just calls down to emitIns_R_R_R_I() with 0 for the immediate. Eg:

runtime/src/coreclr/jit/emitarm64sve.cpp

Line 4377 in 692660e

case INS_sve_ld1b:

Curious...

NI_AdvSimd_Store uses gtNewSimdStoreNode() which eventually gets generated in genCodeForStoreInd(). This has extra code, for example, isvolatile checks.

Meanwhile NI_AdvSimd_StoreSelectedScalar imports like a normal hwintrinsic and gets generated in hwintrinsiccodegenarm64.cpp. This is the same for the various other stores.

Is there a special reason for using genCodeForStoreInd?

Why do the other stores not use genCodeForStoreInd?

Should NI_Sve_Store use genCodeForStoreInd?

I will have to double check on this one. Opened #102347

SwapnilGaikwad · 2024-05-16T13:40:03Z

please run the tests, including the stress tests and share the results

Stress tests didn't flag anything

===================Running default===================
------------------- {} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_float() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_double() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_sbyte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_short() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_int() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_long() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_byte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_ushort() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_uint() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStore_ulong() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_float() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_double() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_sbyte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_short() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_int() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_long() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_byte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_ushort() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_uint() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex2_ulong() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_float() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_double() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_sbyte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_short() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_int() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_long() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_byte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_ushort() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_uint() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex3_ulong() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_float() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_double() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_sbyte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_short() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_int() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_long() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_byte() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_ushort() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_uint() : 7
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.SveStorex4_ulong() : 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'} -------------------

kunalspathak · 2024-05-16T21:41:14Z

As noted in #102180 (comment), these should be StoreAndZip APIs instead of Store APIs because of the operation it is performing.

https://dougallj.github.io/asil/doc/st4d_z_p_bi_64.html

Similar API was named as StoreAndZip in AdvSimd.

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

kunalspathak · 2024-05-16T22:42:42Z

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

+ break;
+ }
+
+ case NI_Sve_Store:


I will have to double check on this one. Opened #102347

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

kunalspathak

LGTM. Thanks!

* Add support for Sve.Store() * Fix formatting issues * Remove incorrect instructions from comment * Rename Sve.Store() -> Sve.StoreAndZip() * Refactor test templates

Add support for Sve.Store()

5f0fe23

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

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

Fix formatting issues

9ed628f

kunalspathak self-requested a review May 15, 2024 17:14

kunalspathak reviewed May 15, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Sve.cs Outdated Show resolved Hide resolved

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

kunalspathak mentioned this pull request May 15, 2024

Arm64: Implement SVE APIs #99957

Open

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

NativeAOT legs timing out in CI #102239

Open

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

a74nh reviewed May 16, 2024

View reviewed changes

SwapnilGaikwad added 2 commits May 16, 2024 13:43

Merge main

23a8653

Remove incorrect instructions from comment

348421f

SwapnilGaikwad marked this pull request as ready for review May 16, 2024 13:21

kunalspathak mentioned this pull request May 16, 2024

SVE: Added Load2xVectorAndUnzip, Load3xVectorAndUnzip, Load4xVectorAndUnzip APIs #102180

Merged

kunalspathak mentioned this pull request May 16, 2024

NI_AdvSimd_Store* use of gtNewSimdStoreNode() #102347

Open

kunalspathak reviewed May 16, 2024

View reviewed changes

SwapnilGaikwad added 2 commits May 21, 2024 15:19

Merge main

4acdbc9

Rename Sve.Store() -> Sve.StoreAndZip()

9d55cd4

Refactor test templates

7aa7eee

This was referenced May 22, 2024

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

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

kunalspathak reviewed May 22, 2024

View reviewed changes

src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveStoreAndZipTest.template Show resolved Hide resolved

kunalspathak approved these changes May 22, 2024

View reviewed changes

kunalspathak merged commit f818517 into dotnet:main May 22, 2024
164 of 167 checks passed

SwapnilGaikwad deleted the github-sve-store branch May 23, 2024 10:20

SwapnilGaikwad mentioned this pull request May 23, 2024

Add support for Sve.StoreNarrowing() #102605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Sve.Store() #102262

Add support for Sve.Store() #102262

SwapnilGaikwad commented May 15, 2024

dotnet-issue-labeler bot commented May 15, 2024

SwapnilGaikwad commented May 15, 2024

dotnet-policy-service bot commented May 15, 2024

kunalspathak left a comment

a74nh May 16, 2024

a74nh May 16, 2024

kunalspathak May 16, 2024

SwapnilGaikwad commented May 16, 2024 •

edited

kunalspathak commented May 16, 2024

kunalspathak May 16, 2024

kunalspathak left a comment

Add support for Sve.Store() #102262

Add support for Sve.Store() #102262

Conversation

SwapnilGaikwad commented May 15, 2024

dotnet-issue-labeler bot commented May 15, 2024

SwapnilGaikwad commented May 15, 2024

dotnet-policy-service bot commented May 15, 2024

kunalspathak left a comment

Choose a reason for hiding this comment

a74nh May 16, 2024

Choose a reason for hiding this comment

a74nh May 16, 2024

Choose a reason for hiding this comment

kunalspathak May 16, 2024

Choose a reason for hiding this comment

SwapnilGaikwad commented May 16, 2024 • edited

kunalspathak commented May 16, 2024

kunalspathak May 16, 2024

Choose a reason for hiding this comment

kunalspathak left a comment

Choose a reason for hiding this comment

SwapnilGaikwad commented May 16, 2024 •

edited