Add support for Sve.Store() #102262

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

Contribute towards #99957.

ghost · 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

ghost 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

Closed

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

NativeAOT legs timing out in CI #102239

Closed

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

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

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

Closed

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

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

github-actions bot locked and limited conversation to collaborators Jun 23, 2024

Add support for Sve.Store() #102262

Add support for Sve.Store() #102262

Uh oh!

Conversation

SwapnilGaikwad commented May 15, 2024

Uh oh!

ghost commented May 15, 2024

Uh oh!

SwapnilGaikwad commented May 15, 2024

Uh oh!

dotnet-policy-service bot commented May 15, 2024

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

a74nh May 16, 2024

Choose a reason for hiding this comment

Uh oh!

a74nh May 16, 2024

Choose a reason for hiding this comment

Uh oh!

kunalspathak May 16, 2024

Choose a reason for hiding this comment

Uh oh!

SwapnilGaikwad commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunalspathak commented May 16, 2024

Uh oh!

Uh oh!

kunalspathak May 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SwapnilGaikwad commented May 16, 2024 •

edited

Loading