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

a74nh · 2023-11-15T10:03:18Z

Many of the files touched here are autogenerated via InstructionSetDesc.txt.

Many of the files touched here are autogenerated via InstructionSetDesc.txt

ghost · 2023-11-15T10:03:26Z

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-15T10:03:30Z

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

Issue Details

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-15T10:04:18Z

Currently includes all of @kunalspathak's #94529. I don't think it's possible to directly base a pull request on top of another pull request. All of my code starts from ceccafa

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 running the included test, it fails with the following error:

Assert failure(PID 2048593 [0x001f4251], Thread: 2048593 [0x1f4251]): Assertion failed 'insOptsNone(id->idInsOpt())' in 'JIT.HardwareIntrinsics.Arm._AdvSimd.SveTest__SveTest:unzip(System.Numerics.Vector`1[int],System.Numerics.Vector`1[int]):System.Numerics.Vector`1[int]:this' during 'Generate code' (IL size 8; hash 0x0bc0f608; Tier0)

    File: /home/alahay01/dotnet/runtime_sve/src/coreclr/jit/emitarm64.cpp Line: 948
    Image: /home/alahay01/dotnet/runtime_sve/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

This is because id->idInsOpt() is set to INS_OPTS_4S. That is set in genHWIntrinsic(). I think we need to add a new flag HW_Category_SIMDVariable to do special handling.

This PR already touches quite a few files, so I was thinking any further codegen changes would go in a separate PR.

@tannergooding @BruceForstall @TamarChristinaArm

a74nh · 2023-11-15T10:14:29Z

Count16BitElements() currently generates to MRS because we're missing all the codegen parts for the CNTH and MRS is a fairly safe instruction to use for now.

I had to do some changes to allow for API functions that don't use any vector registers. Happy to pull this part out of the PR into a new one.

kunalspathak · 2023-11-15T15:10:20Z

Many of the files touched here are autogenerated via InstructionSetDesc.txt.

IMO, lets have a separate PR for these changes and that way, this PR will just be adding the UnzipEven() and Count16BitElements().

a74nh · 2023-11-15T16:49:34Z

Many of the files touched here are autogenerated via InstructionSetDesc.txt.

IMO, lets have a separate PR for these changes and that way, this PR will just be adding the UnzipEven() and Count16BitElements().

Done - #94791

BruceForstall

Hopefully we can start getting some of the "base" PRs merged soon so we'll only be looking at incremental diffs...

BruceForstall · 2023-11-15T20:29:26Z

src/coreclr/jit/emitarm64.cpp

            break;

+        case INS_sve_uzp1:
+            // TODO: This could also be SVE_BR_3B or SVE_CI_3A


We should standardize on TODO-SVE: for marking all SVE-related "to-do" work items.

kunalspathak · 2023-11-16T04:08:13Z

Hopefully we can start getting some of the "base" PRs merged soon so we'll only be looking at incremental diffs...

I agree. I think we should definitely get in #94791 (for InstructionSet related changes). I am under the impression that in order to merge other example PRs like this one or #94529, we need to have register support, so we can validate the encoding. On the contrary, we could start merging the instructions in (probably commented code or some different way) and once register allocation changes are done and we have a tool to validate encoding, we can enable those instructions. Thoughts?

a74nh · 2023-11-16T12:34:55Z

Empty API code is here: #94791
Count16BitElements() is here: #94845

I'll reduce this PR down to just UnzipEven()

BruceForstall · 2023-11-16T20:27:43Z

Thoughts?

I wonder how many regalloc changes we really need to be able to make progress, assuming we don't need to generate predicate registers. Presumably we'd still use Vn registers, and they're encoded just like Zn registers. (Maybe the predicate register encoding could always assume predicate register zero, say, or use a Vn register temporarily instead of a new Pg register.)

kunalspathak · 2023-11-16T20:32:13Z

I wonder how many regalloc changes we really need to be able to make progress, assuming we don't need to generate predicate registers. Presumably we'd still use Vn registers, and they're encoded just like Zn registers.

Yes, for now, the easier change to do is add Zn register so the code gets disassembled properly and then reuse one of the vector register for P0 predicate register.

TamarChristinaArm · 2023-11-17T10:47:22Z

src/coreclr/inc/corinfoinstructionset.h

            return "VectorT128";
        case InstructionSet_Rcpc2 :
            return "Rcpc2";
+        case InstructionSet_Sve :


Curious, why are there two instruction sets here? For the other ones it was because of AArch32, but there's no AArch32 SVE.

AIUI, we need both to matching the existing structure. All the functions will be in Sve and Sve.Arm64 will be empty.

MichalPetryka · 2023-11-20T15:04:39Z

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

+
+    public sealed unsafe class SveTest__SveTest
+    {
+        public bool IsSupported => AdvSimd.IsSupported;


Suggested change

public bool IsSupported => AdvSimd.IsSupported;

public bool IsSupported => Sve.IsSupported;

?

a74nh · 2023-11-24T16:56:33Z

This has been replaced by
#94845 and #95021

kunalspathak and others added 10 commits November 8, 2023 10:11

Add uzp1 in emitOutputInstr()

5584571

Add uzp1 in emitInsSanityCheck()

0a99b64

Add uzp1 in Perscore calculation()

9710738

Add uzp1 in emitDispInsHelp()

80b7c88

Add uzp1 in emitter unit test

bfe9b57

Add uzp1 in MayWriteToGCReg

16f7b13

jit format

4936757

fix the build error

517cfd6

fix the instruction name

72a5613

UnzipEven() and Count16BitElements() intrinsics for SVE

ceccafa

Many of the files touched here are autogenerated via InstructionSetDesc.txt

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

Add empty SVE API for Arm64 #94791

Closed

BruceForstall reviewed Nov 15, 2023

View reviewed changes

a74nh mentioned this pull request Nov 16, 2023

Count16BitElements() intrinsic for SVE #94845

Closed

TamarChristinaArm reviewed Nov 17, 2023

View reviewed changes

MichalPetryka reviewed Nov 20, 2023

View reviewed changes

kunalspathak mentioned this pull request Nov 20, 2023

Arm64/SVE: Add Uzp1 #94529

Closed

a74nh closed this Nov 24, 2023

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

	public bool IsSupported => AdvSimd.IsSupported;
	public bool IsSupported => Sve.IsSupported;

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

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

Uh oh!

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!

kunalspathak commented Nov 15, 2023

Uh oh!

a74nh commented Nov 15, 2023

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

BruceForstall Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented Nov 16, 2023

Uh oh!

a74nh commented Nov 16, 2023

Uh oh!

BruceForstall commented Nov 16, 2023

Uh oh!

kunalspathak commented Nov 16, 2023

Uh oh!

TamarChristinaArm Nov 17, 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!

a74nh Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

MichalPetryka Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

a74nh commented Nov 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TamarChristinaArm Nov 17, 2023 •

edited

Loading