Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Arm64] AddAcross MaxAcross MinAcross MaxNumber MaxNumberPairwise MaxPairwise #32620

Conversation

echesakov
Copy link
Contributor

@echesakov echesakov commented Feb 20, 2020

  1. This PR implements the following intrinsics:
  • AddPairwise, AddPairwiseScalar
  • MaxAcross
  • MaxNumber, MaxNumberScalar
  • MaxNumberAcross
  • MaxPairwise, MaxPairwiseScalar
  • MinAcross
  • MinNumberAcross
  • MinPairwise, MinPairwiseScalar
  1. Updates AddAcross to return Vector64 as we discussed in API review meeting

  2. Updates emitter such that emitIns functions for faddp (scalar), fmaxp (scalar), fmaxv, smaxv and similar instructions require opt argument to have non-INS_OPTS_NONE value (i.e. specified according to the Arm technical documentation even in a case when an instruction has only one valid vector arrangement). For example, 2-operand form of faddp requires INS_OPTS_2D options to be specified and fmaxv requires INS_OPTS_4S.

  3. Changes the precedence of criteria for computing instruction SIMD size for GT_HWINTRINSIC node (7391ecd). Before the change, if an intrinsic returns a struct type value - then the size of the value will be used for determining SIMD size. After the change, if an intrinsic has a flag BaseTypeFromFirstArg or BaseTypeFromSecondArg specified - then the SIMD size of the tree node will be based on the corresponding parameter size (i.e op1 or op2 size). For example, AddAcross always returns Vector64 value and the SIMD size cannot be based on the return value size (since it will be the same for AddAcross(Vector64) and AddAcross(Vector128)).

  4. Moves smaxv, sminv, addv and similar instructions to a separate instruction form DV_2T.

Below commits are grouped by their themes:

System.Private.CoreLib\src\System\Runtime\Intrinsics\Arm

ff93898 Add "MaxAcross" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

4a9c400 Add "MinAcross" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

cae0066 Update "AddAcross" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

9b13e23 Add "AddPairwise" and "AddPairwiseScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

6dcee66 Add "MaxPairwise" and "MaxPairwiseScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

8748dda Add "MinPairwise" and "MinPairwiseScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

3bcdd50 Add "MaxNumber" and "MaxNumberScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

32380ad Add "MaxNumberPairwise" and "MaxNumberPairwiseScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

7128c88 Add "MinNumber" and "MinNumberScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

c285661 Add "MinNumberPairwise" and "MinNumberPairwiseScalar" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

a3c2767 Add "MinNumberAcross" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

b5931c6 Add "MaxNumberAcross" in AdvSimd.cs AdvSimd.PlatformNotSupported.cs

src\libraries\System.Runtime.Intrinsics.Experimental\ref

ad52b4f Update System.Runtime.Intrinsics.Experimental.cs

src\coreclr\src\jit

7391ecd Precedence of BaseTypeFromFirstArg, BaseTypeFromSecondArg over "return type being a struct" in HWIntrinsicInfo::lookupSimdSize in hwintrinsic.cpp

74f3776 Fix comment in instrsarm64.h

a75ec58 Datasize = 16 bytes for fmaxnmv, fmaxv, fminnv, fminv in emitarm64.cpp

58eba4a Datasize = 8 bytes or 16 bytes for faddp, fmaxnmp, fmaxp, fminp, fminp in emitarm64.cpp

c19b26e Datasize = 16 bytes for addp in emitarm64.cpp

e89d6af Update usages of faddp instruction in CodeGen::genSIMDIntrinsicDotProduct in src/jit/codegenarm64.cpp

a115435 Use DV_2T encoding form for addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv in emitarm64.cpp emitfmtsarm64.h instrsarm64.h

b22616e Share IF_DV_2A and IF_DV_2R in emitter::emitOutputInstr in emitarm64.cpp

4abd134 Fix formatting in emitarm64.cpp

b0f9e03 Update "AddAcross" in hwintrinsiclistarm64.h

fbea4f0 Add "MaxAcross" and "MinAcross" in hwintrinsiclistarm64.h

d03e744 Add "AddPairwise" and "AddPairwiseScalar" in hwintrinsiclistarm64.h

4a0d153 Add "MaxPairwise" and "MaxPairwiseScalar" in hwintrinsiclistarm64.h

dd1fe99 Add "MinPairwise" and "MinPairwiseScalar" in hwintrinsiclistarm64.h

c70d14b Order methods alphabetically in hwintrinsiclistarm64.h

2b7f289 Add "MaxNumber" and "MaxNumberScalar" in hwintrinsiclistarm64.h

6efbbbc Add "MinNumber" and "MinNumberScalar" in hwintrinsiclistarm64.h

62de9f7 Add "MaxNumberPairwise" and "MaxNumberPairwiseScalar" in hwintrinsiclistarm64.h

140d435 Add "MinNumberPairwise" and "MinNumberPairwiseScalar" in hwintrinsiclistarm64.h

e8f2ad2 Add "MaxNumberAcross" in hwintrinsiclistarm64.h

bb14f72 Add "MinNumberAcross" in hwintrinsiclistarm64.h

405fc6f Update CodeGen::genArm64EmitterUnitTests() in codegenarm64.cpp

src\coreclr\tests\src\JIT\HardwareIntrinsics\Arm

07eb50b Update "AddAcross" in GenerateTests.csx

7a80cbf Add "MaxAcross" in GenerateTests.csx

3ca6980 Add "MinAcross" in GenerateTests.csx

c91cd6a Add "MaxNumber" and "MaxNumberScalar" in GenerateTests.csx

9c663bd Add "MinNumber" and "MinNumberScalar" in GenerateTests.csx

2b4ade6 Add "MaxNumberAcross" and "MinNumberAcross" in GenerateTests.csx

7fe0318 Add "AddPairwise" in AdvSimd in GenerateTests.csx

7cbef4d Add "AddPairwise" and "AddPairwiseScalar" in AdvSimd.Arm64 in GenerateTests.csx

dfb70f8 Add "MaxPairwise" in AdvSimd in GenerateTests.csx

0573dec Add "MaxPairwise" and "MaxPairwiseScalar" in AdvSimd.Arm64 in GenerateTests.csx

54724f7 Add "MinPairwise" in AdvSimd in GenerateTests.csx

82de0d1 Add "MinPairwise" and "MinPairwiseScalar" in AdvSimd.Arm64 in GenerateTests.csx

808a55d Add "MaxNumberPairwise" and "MaxNumberPairwiseScalar" in AdvSimd.Arm64 in GenerateTests.csx

9acbc2c Add "MinNumberPairwise" and "MinNumberPairwiseScalar" in AdvSimd.Arm64 in GenerateTests.csx

b4d77d5 Update Helpers.cs Helpers.tt

737288a Update AdvSimd/ AdvSimd.Arm64/

e096445 Remove whitespace in _BinaryOpTestTemplate.template _TernaryOpTestTemplate.template _UnaryOpTestTemplate.template

28d9648 Update AdvSimd/ AdvSimd.Arm64/

Part of #24794

…n type being a struct" in HWIntrinsicInfo::lookupSimdSize in hwintrinsic.cpp
…, uminv in emitarm64.cpp emitfmtsarm64.h instrsarm64.h
@echesakov echesakov added arch-arm64 area-System.Runtime.Intrinsics area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Feb 20, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

@echesakov echesakov marked this pull request as ready for review February 21, 2020 01:01
@echesakov
Copy link
Contributor Author

@CarolEidt @tannergooding @TamarChristinaArm PTAL
cc @dotnet/jit-contrib

@echesakov
Copy link
Contributor Author

@briansull I changed the emitter for fmaxv, faddp, smax etc as we discussed. Can you please review these changes?

Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The emitter changes look good

@@ -4516,13 +4516,13 @@ void CodeGen::genSIMDIntrinsicDotProduct(GenTreeSIMD* simdNode)
{
if (opt == INS_OPTS_4S)
{
GetEmitter()->emitIns_R_R_R(INS_faddp, attr, tmpReg, tmpReg, tmpReg, INS_OPTS_4S);
GetEmitter()->emitIns_R_R_R(INS_faddp, EA_16BYTE, tmpReg, tmpReg, tmpReg, INS_OPTS_4S);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just changing the size from element to vector right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly - my previous change #32277 that did the opposite for faddp (i.e. changing the size from vector to element) was breaking consistency with other part of the JIT and I am rolling it back here.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As usual, reviewed the JIT, Corelib, and test template changes, but only looked at a couple of the generated tests.

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@echesakov
Copy link
Contributor Author

@tannergooding Should I remove label new-api-needs-documentation since I documented all the newly added functions here? Or you will use for some bookkeeping at the later point?

@tannergooding
Copy link
Member

I'm fine with it being removed provided everything was documented.

@echesakov echesakov merged commit 9981b1f into dotnet:master Feb 21, 2020
@echesakov echesakov deleted the Arm64-Add-Max-Min-Across-Number-NumberPairwise-Pairwise branch February 21, 2020 20:22
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI area-System.Runtime.Intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants