[Arm64] ASIMD DotProduct and Rounding Double Multiply Add/Subtract #38957

echesakov · 2020-07-08T21:04:10Z

DotProduct
DotProductBySelectedQuadruplet
MultiplyRoundedDoublingAndAddSaturateHigh
MultiplyRoundedDoublingAndAddSaturateHighScalar
MultiplyRoundedDoublingAndSubtractSaturateHigh
MultiplyRoundedDoublingAndSubtractSaturateHighScalar
MultiplyRoundedDoublingBySelectedScalarAndAddSaturateHigh
MultiplyRoundedDoublingBySelectedScalarAndSubtractSaturateHigh
MultiplyRoundedDoublingScalarBySelectedScalarAndAddSaturateHigh
MultiplyRoundedDoublingScalarBySelectedScalarAndSubtractSaturateHigh

With help from @eiriktsarpalis I collected the test results on an Arm64 device that supports these ISAs - I attached the archive with the files.

Dp_Rdm_Rdm.Arm64.zip

Dotnet-GitSync-Bot · 2020-07-08T21:04:17Z

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, to 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 · 2020-07-09T00:55:04Z

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

echesakov · 2020-07-09T18:52:30Z

@CarolEidt @tannergooding @TamarChristinaArm This is ready for review, please take a look.

tannergooding · 2020-07-09T20:31:24Z

src/coreclr/src/inc/corinfoinstructionset.h

@@ -28,8 +28,11 @@ enum CORINFO_InstructionSet
    InstructionSet_Sha1=8,
    InstructionSet_Sha256=9,
    InstructionSet_Atomics=10,
-    InstructionSet_Vector64=11,
-    InstructionSet_Vector128=12,
+    InstructionSet_Dp=11,


Dp_Arm64 should also exist, it isn't covered by #38460

Okay, I will update/rebase this PR after you merge #38460

CarolEidt

This LGTM overall, with one question. It seems that you and @tannergooding will have to coordinate with his #38460 (i.e. one will have to merge)

CarolEidt · 2020-07-09T20:28:58Z

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

+        public static new bool IsSupported { get => IsSupported; }
+
+        [Intrinsic]
+        public new abstract class Arm64 : AdvSimd.Arm64


I'm confused why this is here if there are no 64-bit intrinsics in this class.

I added this as a placeholder and I anticipated to wait till Tanner merges his PR and update thereafter. I believe this is needed to be consistent with #34587.

My understanding that Dp.Arm64.IsSupported needs to return false if Dp.IsSupported is false.
While now Dp.Arm64.IsSupported returns true if AdvSimd.Arm64.IsSupported is true, so we need to treat Dp.Arm64.IsSupported as intrinsic instead of relying on it being inherited from AdvSimd.Arm64.

Dp.Arm64.IsSupported needs to return true if Dp.IsSupported and we are in a 64-bit process. It being true implies Dp.IsSupported and ArmBase.Arm64.IsSupported (which each also implies ArmBase.IsSupported).

Having it recursive here and adding the relevant entry to Arm64VersionOfIsa should hook it all up correctly (same as was done for Rdm: https://github.com/dotnet/runtime/pull/38957/files#diff-8e07abb3dff9bfe2e051a071c787f18bR27)

It exists because of #34587 (and is the same reason as #38460 was created), which is that we otherwise get into weird scenarios where Dp.Arm64.IsSupported can return true (because it actually resolves to ArmBase.Arm64.IsSupported when Dp.IsSupported returns false.

@CarolEidt Do you have any other questions here?

No, makes sense (sort of!)

tannergooding · 2020-07-09T20:35:05Z

src/coreclr/src/pal/src/misc/jitsupport.cpp

@@ -50,8 +50,8 @@ PAL_GetJitCpuCapabilityFlags(CORJIT_FLAGS *flags)
 //        CPUCompileFlags.Set(CORJIT_FLAGS::CORJIT_FLAG_HAS_ARM64_DCPOP);
 #endif
 #ifdef HWCAP_ASIMDDP
-//    if (hwCap & HWCAP_ASIMDDP)
-//        CPUCompileFlags.Set(CORJIT_FLAGS::CORJIT_FLAG_HAS_ARM64_DP);
+    if (hwCap & HWCAP_ASIMDDP)


We should add a tracking issue with the tracking-external-issue label for when Windows support is in, so we can update the codeman checks for Windows.

Done #39094

…cs in System.Private.CoreLib.Shared.projitems

…m64.cpp

…rm64.h

…lingAndSubtractSaturateHigh in hwintrinsiclistarm64.h

…lingAndSubtractSaturateHigh in GenerateTests.csx

…istarm64.h

tannergooding · 2020-07-10T19:21:26Z

src/coreclr/src/jit/hwintrinsicarm64.cpp

@@ -140,20 +158,19 @@ bool HWIntrinsicInfo::isFullyImplementedIsa(CORINFO_InstructionSet isa)
        case InstructionSet_ArmBase_Arm64:
        case InstructionSet_Crc32:
        case InstructionSet_Crc32_Arm64:
+        case InstructionSet_Dp:


Missing Dp_Arm64?

tannergooding · 2020-07-10T19:26:22Z

It would be good to update JIT/Regression/JitBlue/Runtime_34587 to validate the hierarchy of the new ISAs

echesakov · 2020-07-10T19:39:26Z

It would be good to update JIT/Regression/JitBlue/Runtime_34587 to validate the hierarchy of the new ISAs

It looks that this tests was merged in the wrong place (under src/coreclr/tests/src instead of src/tests) - I will move it and update as you suggested

echesakov · 2020-07-10T19:49:38Z

@tannergooding Updated the test. Unfortunatelly, it does not do anything in CI now since our machines don't support Dp and Rdm

tannergooding · 2020-07-10T20:55:31Z

Unfortunatelly, it does not do anything in CI now since our machines don't support Dp and Rdm

That's unfortunate, but possibly something we can address longer term. This scenario will be an issue (in the future) as we start onboarding newer ISAs

echesakov · 2020-07-13T18:48:52Z

@tannergooding I am going to merge this to unblock the remaining hardware intrinsics work.
Please let me know, if you want me to follow up on anything else here.

Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Jul 8, 2020

echesakov added arch-arm64 area-System.Runtime.Intrinsics and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Jul 9, 2020

echesakov force-pushed the Arm64-ASIMD-DotProduct-Rounding-Double-Multiply-Add-Subtract branch from 55e7c8b to 478fef1 Compare July 9, 2020 02:46

echesakov marked this pull request as ready for review July 9, 2020 18:51

tannergooding reviewed Jul 9, 2020

View reviewed changes

CarolEidt approved these changes Jul 9, 2020

View reviewed changes

tannergooding reviewed Jul 9, 2020

View reviewed changes

echesakov added 16 commits July 10, 2020 11:08

Add Dp.cs Dp.PlatformNotSupported.cs

6c9bc75

Add Rdm.cs Rdm.PlatformNotSupported.cs

b84568f

Add Dp.cs Dp.PlatformNotSupported.cs Rdm.cs Rdm.PlatformNotSupported.…

6bb15f5

…cs in System.Private.CoreLib.Shared.projitems

Update System.Runtime.Intrinsics.cs

0fbb96a

Update PAL_GetJitCpuCapabilityFlags() in jitsupport.cpp

57dcd4f

Add InstructionSetDesc.txt

4d7b63c

Update jitinterface files

75ef984

Recognize Dp and Rdm classes as intrinsic containers in hwintrinsicar…

c48df72

…m64.cpp

Add DotProduct and DotProductBySelectedQuadruplet in hwintrinsiclista…

fb6851a

…rm64.h

Add MultiplyRoundedDoublingAndAddSaturateHigh and MultiplyRoundedDoub…

7df470b

…lingAndSubtractSaturateHigh in hwintrinsiclistarm64.h

Add DotProduct and DotProductBySelectedQuadruplet in GenerateTests.csx

c7cc928

Add MultiplyRoundedDoublingAndAddSaturateHigh and MultiplyRoundedDoub…

f08bd97

…lingAndSubtractSaturateHigh in GenerateTests.csx

Whitespace formatting in GenerateTests.csx

ba0f058

Update Helpers.cs Helpers.tt

8001bdc

Update Shared/Program.cs

feb1bab

Add Dp/ Rdm/ Rdm.Arm64/

3313eb8

echesakov added 4 commits July 10, 2020 11:28

Update HardwareIntrinsics/Arm/

29da009

Update DotProductBySelectedQuadruplet in hwintrinsic.cpp hwintrinsicl…

1e9ea9b

…istarm64.h

Exclude Rdm, Rdm.Arm64 and Dp tests from Mono in issues.targets

2c46689

Fix Dp.PlatformNotSupported.cs

bc08993

echesakov force-pushed the Arm64-ASIMD-DotProduct-Rounding-Double-Multiply-Add-Subtract branch from 478fef1 to bc08993 Compare July 10, 2020 18:29

echesakov mentioned this pull request Jul 10, 2020

[win-arm64] Update EEJitManager::SetCpuInfo in codeman.cpp to support ARMv8.1-RDMA and ARMv8.2-DotProd #39094

Open

tannergooding reviewed Jul 10, 2020

View reviewed changes

Update HWIntrinsicInfo::isFullyImplementedIsa() in hwintrinsicarm64.cpp

6ba0b1b

echesakov added 2 commits July 10, 2020 12:43

Move Runtime_34587/ to src/tests/JIT/Regression/JitBlue/

3842467

Update Runtime_34587 with Dp and Rdm

a8bb36a

echesakov requested a review from tannergooding July 13, 2020 15:47

echesakov merged commit d9a7d8f into dotnet:master Jul 13, 2020

echesakov deleted the Arm64-ASIMD-DotProduct-Rounding-Double-Multiply-Add-Subtract branch July 13, 2020 18:49

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Arm64] ASIMD DotProduct and Rounding Double Multiply Add/Subtract #38957

[Arm64] ASIMD DotProduct and Rounding Double Multiply Add/Subtract #38957

echesakov commented Jul 8, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Jul 8, 2020

ghost commented Jul 9, 2020

echesakov commented Jul 9, 2020

tannergooding Jul 9, 2020

echesakov Jul 9, 2020

echesakov Jul 10, 2020

CarolEidt left a comment

CarolEidt Jul 9, 2020

echesakov Jul 9, 2020 •

edited

Loading

echesakov Jul 9, 2020 •

edited

Loading

tannergooding Jul 9, 2020

echesakov Jul 10, 2020

CarolEidt Jul 10, 2020

tannergooding Jul 9, 2020

echesakov Jul 10, 2020

tannergooding Jul 10, 2020

echesakov Jul 10, 2020

tannergooding commented Jul 10, 2020

echesakov commented Jul 10, 2020

echesakov commented Jul 10, 2020

tannergooding commented Jul 10, 2020

echesakov commented Jul 13, 2020

[Arm64] ASIMD DotProduct and Rounding Double Multiply Add/Subtract #38957

[Arm64] ASIMD DotProduct and Rounding Double Multiply Add/Subtract #38957

Conversation

echesakov commented Jul 8, 2020 • edited Loading

Dotnet-GitSync-Bot commented Jul 8, 2020

ghost commented Jul 9, 2020

echesakov commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echesakov Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

echesakov Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Jul 10, 2020

echesakov commented Jul 10, 2020

echesakov commented Jul 10, 2020

tannergooding commented Jul 10, 2020

echesakov commented Jul 13, 2020

echesakov commented Jul 8, 2020 •

edited

Loading

echesakov Jul 9, 2020 •

edited

Loading

echesakov Jul 9, 2020 •

edited

Loading