ARM64-SVE: Add `TrigonometricMultiplyAddCoefficient` by amanasifkhalid · Pull Request #104697 · dotnet/runtime

amanasifkhalid · 2024-07-10T20:48:40Z

Part of #99957. @dotnet/arm64-contrib PTAL, thanks!

Test output:

Starting test: .\Core_Root\corerun.exe .\HardwareIntrinsics_Arm_r\HardwareIntrinsics_Arm_r.dll Sve_TrigonometricMultiplyAddCoefficient
===================Running default===================
------------------- {} -------------------
Passed test: _Sve_r::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_TrigonometricMultiplyAddCoefficient_float() : 31
Passed test: _Sve_r::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_TrigonometricMultiplyAddCoefficient_double() : 31
===================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'} -------------------

Starting test: .\Core_Root\corerun.exe .\HardwareIntrinsics_Arm_ro\HardwareIntrinsics_Arm_ro.dll Sve_TrigonometricMultiplyAddCoefficient
===================Running default===================
------------------- {} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_TrigonometricMultiplyAddCoefficient_float() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_TrigonometricMultiplyAddCoefficient_double() : 31
===================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'} -------------------

ghost · 2024-07-10T20:48:46Z

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 · 2024-07-10T20:48:47Z

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.

dotnet-policy-service · 2024-07-10T20:49:19Z

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

kunalspathak

Added some questions around test.

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

kunalspathak · 2024-07-11T05:15:12Z

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

            public void RunStructFldScenario({TemplateName}BinaryOpTest__{TestName} testClass)
            {
-                var result = {Isa}.{Method}(_fld1, _fld2, {Imm});
+                var result = {Isa}.{Method}(_fld1, _fld2, Imm);


This is missing a test where we pass invalidImm as input. See test.RunBasicScenario_UnsafeRead_InvalidImm in _SveImmUnaryOpTestTemplate.template for example.

Got it. I noticed the InvalidImm tests in other templates look like this:

bool succeeded = false; try { var result = {Isa}.{Method}( Unsafe.Read<{Op1VectorType}<{Op1BaseType}>>(_dataTable.inArrayPtr), {InvalidImm} ); } catch (ArgumentOutOfRangeException) { succeeded = true; }

We aren't doing anything with succeeded. Is this intentional?

We aren't doing anything with succeeded. Is this intentional?

Unfortunately, it is not. It is missing this code:

if (!succeeded) { Succeeded = false; }

Opened #104809 to track it. Feel free to just update this one and for others we will take care of as part of fixing the overall issue.

kunalspathak · 2024-07-11T05:15:52Z

src/tests/JIT/HardwareIntrinsics/Arm/Shared/Helpers.cs

+        public static float TrigonometricMultiplyAddCoefficient(float op1, float op2, byte imm)
+        {
+            int index = (op2 < 0) ? (imm + 8) : imm;
+            uint coeff = index switch


@SwapnilGaikwad - can you please confirm this logic verifies the operation of the instruction?

src/tests/JIT/HardwareIntrinsics/Arm/Shared/Helpers.cs

kunalspathak · 2024-07-12T16:12:25Z

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

                                        Unsafe.Read<{Op1VectorType}<{Op1BaseType}>>(_dataTable.inArray1Ptr),
                                        Unsafe.Read<{Op1VectorType}<{Op1BaseType}>>(_dataTable.inArray2Ptr),
-                                        (byte){Imm}
+                                        (byte)Imm


By having Imm, we will always generate table driven logic, while with {Imm}, we will always generate the single instruction, with constant embedded. I think we need to have a test for both and I have captured it in #104809. Please revert these changes and we will address them together as part of that issue.

Got it, I did this out of convenience for writing the test definitions: If we do {Imm}, then we cannot dynamically generate the immediate value using something like TestLibrary.Generator.GetByte() because the value will change each time we use {Imm}. Instead, we'll have to hard-code the immediate into the test with something like ["Imm"] = "0". I don't mind doing this; it will just result in more tests for this API.

kunalspathak · 2024-07-12T16:13:47Z

src/tests/JIT/HardwareIntrinsics/Arm/Shared/Helpers.cs


+        public static float TrigonometricMultiplyAddCoefficient(float op1, float op2, byte imm)
+        {
+            int index = (op2 < 0) ? (imm + 8) : imm;


have we verified the behavior of this API with invalid values? If it is undefined, are we making the test robust to handle it?

Like in #104681, the tests seem to work fine with out-of-bound values, though I can easily modify the tests to skip validation for such values.

I liked the way you have combined the table of sin and cos in a single lookup.

Like in #104681, the tests seem to work fine with out-of-bound values, though I can easily modify the tests to skip validation for such values.

That will be great.

I liked the way you have combined the table of sin and cos in a single lookup.

Thanks, I wish I could say that was my idea, but the ARM docs suggested it

amanasifkhalid · 2024-07-12T22:08:44Z

@kunalspathak I addressed your feedback. I had to add a usage of the intrinsic's result in the InvalidImm scenario so that the intrinsic itself doesn't get optimized away and fail the test. Stress tests are passing.

kunalspathak

Overall looks good. please make sure we skip the validation for invalid input as well as make sure the other tests using the modified template passes.

kunalspathak · 2024-07-13T14:15:06Z

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

                test.ConditionalSelect_ZeroOp();
+
+                // Validates basic functionality fails with an invalid imm, using Unsafe.ReadUnaligned
+                test.RunBasicScenario_UnsafeRead_InvalidImm();


This template is used by *MultiplyBySelectedScalar* too. Have you verified if those tests pass, including the stress?

kunalspathak · 2024-07-13T14:21:21Z

src/tests/JIT/HardwareIntrinsics/Arm/Shared/Helpers.cs


+        public static float TrigonometricMultiplyAddCoefficient(float op1, float op2, byte imm)
+        {
+            int index = (op2 < 0) ? (imm + 8) : imm;


I liked the way you have combined the table of sin and cos in a single lookup.

kunalspathak · 2024-07-13T14:21:51Z

src/tests/JIT/HardwareIntrinsics/Arm/Shared/Helpers.cs


+        public static float TrigonometricMultiplyAddCoefficient(float op1, float op2, byte imm)
+        {
+            int index = (op2 < 0) ? (imm + 8) : imm;


Like in #104681, the tests seem to work fine with out-of-bound values, though I can easily modify the tests to skip validation for such values.

That will be great.

amanasifkhalid · 2024-07-15T14:37:58Z

@kunalspathak I merged from main and reran the stress tests for MultiplyBySelectedScalar and TrigonometricMultiplyAddCoefficient, and they're passing. Is this ready to be merged?

kunalspathak

LGTM. Thanks!

Add ftmad

05cf250

ghost added area-System.Runtime.Intrinsics new-api-needs-documentation labels Jul 10, 2024

dotnet-policy-service bot assigned amanasifkhalid Jul 10, 2024

amanasifkhalid mentioned this pull request Jul 10, 2024

Arm64: Implement SVE APIs #99957

Closed

amanasifkhalid added the arm-sve Work related to arm64 SVE/SVE2 support label Jul 10, 2024

kunalspathak suggested changes Jul 11, 2024

View reviewed changes

Add InvalidImm scenario

a7ede96

kunalspathak mentioned this pull request Jul 12, 2024

Arm64/Sve: Missing test coverage for invalid immediate #104809

Closed

kunalspathak reviewed Jul 12, 2024

View reviewed changes

amanasifkhalid added 3 commits July 12, 2024 16:03

Merge from main

3051e01

Feedback

e07e664

Add side effect to avoid removing InvalidImm call

95dc173

kunalspathak suggested changes Jul 13, 2024

View reviewed changes

Merge from main

2b2594d

kunalspathak approved these changes Jul 15, 2024

View reviewed changes

amanasifkhalid merged commit 5354e0f into dotnet:main Jul 15, 2024

amanasifkhalid deleted the sve-ftmad branch July 15, 2024 16:57

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

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

Closed

github-actions bot locked and limited conversation to collaborators Aug 15, 2024

Conversation

amanasifkhalid commented Jul 10, 2024

Uh oh!

ghost commented Jul 10, 2024

Uh oh!

ghost commented Jul 10, 2024

Uh oh!

dotnet-policy-service bot commented Jul 10, 2024

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!

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!

amanasifkhalid commented Jul 12, 2024

Uh oh!

kunalspathak 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!

amanasifkhalid commented Jul 15, 2024

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants