Add empty SVE API for Arm64 by a74nh · Pull Request #94791 · dotnet/runtime

a74nh · 2023-11-15T16:44:10Z

Adds empty System/Runtime/Intrinsics/Arm/Sve.cs and hwintrinsiclistarm64sve.h.
Plus all connecting code.
Many of the files touched here are autogenerated via InstructionSetDesc.txt.

ghost · 2023-11-15T16:44: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.

ghost · 2023-11-15T16:44:23Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Adds empty System/Runtime/Intrinsics/Arm/Sve.cs and hwintrinsiclistarm64sve.h.
Plus all connecting code.
Many of the files touched here are autogenerated via InstructionSetDesc.txt.

Author:	a74nh
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `new-api-needs-documentation`, `community-contribution`
Milestone:	-

a74nh · 2023-11-15T16:46:37Z

Extracted from #94765
Currently includes all of @kunalspathak's #94529

Not sure if the Arm64_Sve parts are required or if it should only be Sve. I think we want the readytorun/crossgen changes too (but it should be disabled for them?)

When run on a Arm64 Neoverse V1, the test program correctly outputs "SVE is supported."

a74nh · 2023-11-15T16:46:54Z

@tannergooding @BruceForstall

a74nh · 2023-11-15T18:14:17Z

Rebased off head as Kunal's work isn't dependant on this work.

kunalspathak

Early version looks good. Added some questions.

src/coreclr/inc/corinfoinstructionset.h

kunalspathak · 2023-11-15T18:28:08Z

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

+    /// </summary>
+    [Intrinsic]
+    [CLSCompliant(false)]
+    public abstract class Sve : AdvSimd


what is an example of APIs that goes in Sve vs. Sve.Arm64? I thought we wanted something like System.Runtime.Instrinsics.Arm.Sve and System.Runtime.Instrinsics.Arm.Sve2?

The general setup is that Isa exists to hold APIs that work on any arm platform

Isa.Arm32 exists to hold APIs that only work on Arm32
Isa.Arm64 exists to hold APIs that only work on Arm64 (we then have Isa.X64 for xarch)

SVE is interesting because there are many APIs that "could" work on Arm32, but there are currently no plans (based on a prior discussion on one of the proposals) to expose that support there.

This means Arm64 may be empty, but it must exist for usability reasons since Sve inherits from AdvSimd and otherwise Sve.Arm64.IsSupported would resolve to AdvSimd.Arm64.IsSupported and would lead users into a pit of failure, thinking it meant Sve.IsSupported would also be true

ok, I didn't pay attention to : AdvSimd portion actually. I am wondering why Sve and Sve.Arm64 do not inherit from ArmBase and ArmBase.Arm64 respectively? I assume there will be a separate class for Sve2 next to Sve class.

SVE inherits from AdvSimd because it requires AdvSimd support to exist and so it makes it overall more convenient for the user

AdvSimd then inherits from ArmBase itself

that makes sense.

kunalspathak · 2023-11-15T18:29:37Z

src/coreclr/inc/corinfoinstructionset.h

            AddInstructionSet(InstructionSet_Sha1_Arm64);
        if (HasInstructionSet(InstructionSet_Sha256))
            AddInstructionSet(InstructionSet_Sha256_Arm64);
+        if (HasInstructionSet(InstructionSet_Sve))


same here, want to understand difference between InstructionSet_Sve vs. InstructionSet_Sve_Arm64 instead of InstructionSet_Sve and InstructionSet_Sve2.

It doesn't look like this PR is adding Sve2

sure, what I was trying to get is that since we have generated boiler plate code for encodings, etc. of both SVE and SVE2 instructions and we should define SVE2 as well (instead of doing it in future PR).

There is a ton of work to do and we should really focus on finishing SVE before we start on Sve2, IMO

We don’t want to be in a scenario where we leave Sve2 incomplete and half done because we didn’t have enough time for everything

Alternative suggestion: We should focus first on the the instructions that give extra functionality that are not in NEON and will be useful for the existing SDK libraries. That may include some SVE2 instructions. Doing that would ensure some performance uplift for .NET9.

On my TODO list is to rewrite some of the SDK routines in SVE and see if I can find any performance improvements. Then use that to prioritise the SVE work.

It would definitely be helpful to know what that list is.

Having an incomplete ISA is generally something we try to avoid as it can lead to issues when exposing APIs later, missed functionality, etc.

kunalspathak · 2023-11-15T18:31:44Z

src/tests/JIT/HardwareIntrinsics/Arm/Sve/Program.cs

+
+            if (test.IsSupported)
+            {
+                Console.WriteLine("SVE is supported.");


did you verify on SVE hardware if the CPU flags are returned and set correctly?

Yes - both with and without:

Graviton 3:

❯ $CORE_ROOT/corerun /home/alahay01/dotnet/runtime_sve/artifacts/tests/coreclr/linux.arm64.Release/JIT/HardwareIntrinsics/Arm/Sve/Sve/Sve.dll
SVE is supported.

Altra:

❯ $CORE_ROOT/corerun /home/alahay01/dotnet/runtime_sve/artifacts/tests/coreclr/linux.arm64.Release/JIT/HardwareIntrinsics/Arm/Sve/Sve/Sve.dll
SVE is not supported.

src/coreclr/tools/Common/Compiler/HardwareIntrinsicHelpers.cs

a74nh · 2023-11-15T18:47:04Z

2023-11-15T18:21:52.2206448Z /__w/1/s/src/native/minipal/cpufeatures.c:359:17: error: use of undeclared identifier 'HWCAP_SVE'

What OS and version are the builds run on? Surprised this does not exist.

tannergooding

LGTM, but we can't merge until after API review has happened.

BruceForstall · 2023-11-15T20:10:05Z

What OS and version are the builds run on? Surprised this does not exist.

All .NET Linux builds are done inside a CBL-Mariner docker container. You can see the details if you look at the "Initializing containers" step of the build, e.g.: https://dev.azure.com/dnceng-public/public/_build/results?buildId=471209&view=logs&jobId=3f327aea-bfb2-5154-7b3c-2691dcdd6bb2&j=3f327aea-bfb2-5154-7b3c-2691dcdd6bb2&t=aa810107-5ca4-4707-8689-54adfbc0a8ea. In this case, it looks like the container is mcr.microsoft.com/dotnet-buildtools/prereqs:cbl-mariner-2.0-cross-biarch-amd64-arm64.

@sbomer is the expert on these: @sbomer or @a74nh do you know why Mariner might be missing the HWCAP_SVE declaration?

BruceForstall · 2023-11-15T20:13:12Z

Looks like maybe SVE support should have shown up in Linux kernels from 5.15 in around 2017? https://www.kernel.org/doc/html/v5.15/arm64/sve.html

(also, https://docs.kernel.org/next/arm64/elf_hwcaps.html)

a74nh · 2023-11-16T09:41:04Z

Looks like maybe SVE support should have shown up in Linux kernels from 5.15 in around 2017? https://www.kernel.org/doc/html/v5.15/arm64/sve.html

(also, https://docs.kernel.org/next/arm64/elf_hwcaps.html)

Mariner is used for WSL. It's possible this has been disabled due to Windows not yet supporting SVE.

I'll add the following:

#ifndef HWCAP_SVE
#define HWCAP_SVE	(1 << 22)
#endif

That's safe as the kernel is never going to return that value on Mariner.

It's also what we did in gdb and also one of the utilities in the Linux kernel is doing is too.

a74nh · 2023-11-16T09:51:02Z

LGTM, but we can't merge until after API review has happened.

To get this merged, we only need to get one piece of the API reviewed?

kunalspathak · 2023-11-21T18:21:46Z

With InstructionSet changes in a separate PR #95021 , this PR just adds the empty hwintrinsiclistarm64sve.h then?

a74nh · 2023-11-22T10:01:29Z

With InstructionSet changes in a separate PR #95021 , this PR just adds the empty hwintrinsiclistarm64sve.h then?

Done!

a74nh · 2023-11-22T16:05:54Z

After discussion with @kunalspathak, this PR is mostly pointless now. Closing.

ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Nov 15, 2023

a74nh mentioned this pull request Nov 15, 2023

UnzipEven() and Count16BitElements() intrinsics for Arm64 SVE #94765

Closed

Add empty SVE API for Arm64

4ad982f

a74nh force-pushed the empty_api_github branch from 3379c17 to 4ad982f Compare November 15, 2023 18:13

kunalspathak suggested changes Nov 15, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 15, 2023

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 15, 2023

tannergooding approved these changes Nov 15, 2023

View reviewed changes

tannergooding added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 15, 2023

build-analysis bot mentioned this pull request Nov 16, 2023

Wasm.Build.Tests.WasmTemplateTests.RunWithDifferentAppBundleLocations times out #94821

Closed

Add HWCAP_SVE define if it does not exist

d104a4a

a74nh mentioned this pull request Nov 16, 2023

Count16BitElements() intrinsic for SVE #94845

Closed

Remove all API changes

acaf31e

a74nh mentioned this pull request Nov 20, 2023

Add SVE InstructionSet for Arm64 #95021

Merged

build-analysis bot mentioned this pull request Nov 21, 2023

System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783

Closed

Remove InstructionSet additions

47a4c25

a74nh closed this Nov 22, 2023

github-actions bot locked and limited conversation to collaborators Dec 23, 2023

Conversation

a74nh commented Nov 15, 2023

Uh oh!

ghost commented Nov 15, 2023

Uh oh!

ghost commented Nov 15, 2023

Uh oh!

a74nh commented Nov 15, 2023

Uh oh!

a74nh commented Nov 15, 2023

Uh oh!

a74nh commented Nov 15, 2023

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

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!

tannergooding Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

a74nh commented Nov 15, 2023

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Nov 15, 2023

Uh oh!

BruceForstall commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a74nh commented Nov 16, 2023

Uh oh!

a74nh commented Nov 16, 2023

Uh oh!

kunalspathak commented Nov 21, 2023

Uh oh!

a74nh commented Nov 22, 2023

Uh oh!

a74nh commented Nov 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tannergooding Nov 16, 2023 •

edited

Loading

BruceForstall commented Nov 15, 2023 •

edited

Loading