[mono] Intrinsify multiple LoadVector API's by fanyang-mono · Pull Request #98077 · dotnet/runtime

fanyang-mono · 2024-02-07T03:59:07Z

Contributes to #93081

This PR intrinsify the following API's:

AdvSimd.LoadVector64x*
AdvSimd.Arm64.LoadVector128x*
AdvSimd.LoadVector64x*AndUnzip
AdvSimd.Arm64.LoadVector128x*AndUnzip
AdvSimd.LoadAndReplicateToVector64x*
AdvSimd.Arm64.LoadAndReplicateToVector128x*

ghost · 2024-02-07T03:59:12Z

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

Issue Details

null

Author:	fanyang-mono
Assignees:	-
Labels:	`area-Codegen-Intrinsics-mono`
Milestone:	-

ghost · 2024-02-07T03:59:14Z

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.

fanyang-mono · 2024-02-07T22:40:18Z

Mostly working. There is some issue with emitting insertvalue, which I am still working on.

kunalspathak · 2024-02-08T15:28:57Z

cc: @a74nh @SwapnilGaikwad

src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs

fanyang-mono · 2024-02-08T17:44:03Z

Mostly working. There is some issue with emitting insertvalue, which I am still working on.

This has been resolved. The PR is ready to review.

kunalspathak · 2024-02-08T18:37:52Z

@fanyang-mono - can you trigger a pipeline that run these tests under mono? I think it was extra-platform or something?

...ies/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.PlatformNotSupported.cs

src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs

kunalspathak · 2024-02-08T18:48:29Z

src/mono/mono/mini/mini-ops.h

 MINI_OP(OP_ARM64_LDP, "arm64_ldp", VREG, IREG, NONE)
 MINI_OP(OP_ARM64_LDP_SCALAR, "arm64_ldp_scalar", VREG, IREG, NONE)

+MINI_OP(OP_ARM64_LDM, "arm64_ldm", VREG, IREG, NONE)


are these custom names specific for mono? Can we write a comment for them because there will be more coming for store? Also, should arm64_ldm reused for LoadVector* and LoadAndUnzip?

All the custom names used in this file are specific for Mono IR. For LLVM codegen, they follows similar logic, so I use the same string for both LoadVector* and LoadAndUnzip

kunalspathak · 2024-02-08T18:49:58Z

src/mono/mono/mini/mini-llvm.c

 						 */
 						 LLVMTypeRef associated_type = intrin_types [vw][0];
 						 intrins = add_intrins2 (module, id, distinguishing_type, associated_type, &intrins_type);
+					} else if (kind == INTRIN_kind_add_pointer) {


what is this change about and why we have to track this separately? Mono supported APIs that takes pointers, right?

This is related to calling LLVM intrinsics. Yes, Mono supports pointer type.

steveisok · 2024-02-09T15:32:46Z

@fanyang-mono build failures are related. This is one from tvos-arm64

mini-llvm.c:11641:45: error: incompatible pointer types passing 'MonoLLVMModule *' to parameter of type 'EmitContext *' [-Werror,-Wincompatible-pointer-types]
                                  addresses [ins->dreg] = create_address (ctx->module, build_named_alloca (ctx, m_class_get_byval_arg (ins->klass), oname), ret_t);
                                                                          ^~~~~~~~~~~
  /Users/runner/work/1/s/src/mono/mono/mini/mini-llvm.c:616:30: note: passing argument to parameter 'ctx' here
  create_address (EmitContext *ctx, LLVMValueRef value, LLVMTypeRef type)

fanyang-mono · 2024-02-09T15:55:37Z

The latest commit should be able to resolve this build failure.

fanyang-mono · 2024-02-09T16:06:25Z

/azp run runtime-llvm

kunalspathak · 2024-02-09T16:32:57Z

Why not also implement LoadVectorAndInsertScalar in this PR and get done with all the load APIs?

fanyang-mono · 2024-02-09T19:27:15Z

CI failures are known.

fanyang-mono · 2024-02-12T16:40:25Z

Why not also implement LoadVectorAndInsertScalar in this PR and get done with all the load APIs?

The LLVM intrinsic that needs to be emitted is a little different. I need more time to make sure everything around it works properly. This PR is ready to be merged. Thus, I would prefer to get this one merged soon.

kunalspathak

LGTM, just make sure the tests you enabled ran and passed on mono.

src/mono/mono/mini/simd-intrinsics.c

src/mono/mono/mini/mini-llvm.c

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

src/mono/mono/mini/mini-llvm.c

fanyang-mono · 2024-02-15T21:34:33Z

CI lane (Mono full AOT with LLVM running runtime tests on arm64) which could validate this change is currently off. Here is the result from local run on my machine:

Result from local run:
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        False
  Rdm:       False
  Sha1:      True
  Sha256:    True
  Sve:       False
 
.......
 
Expected: 100
Actual: 100
END EXECUTION - PASSED

Intrinsify LoadVector*

b532aaa

fanyang-mono added the area-Codegen-Intrinsics-mono label Feb 7, 2024

ghost added the new-api-needs-documentation label Feb 7, 2024

ghost assigned fanyang-mono Feb 7, 2024

fanyang-mono requested a review from steveisok February 7, 2024 03:59

This was referenced Feb 7, 2024

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049

Closed

fanyang-mono added 3 commits February 7, 2024 15:40

Fix the support for v128

c7ce20a

Add logic to handle tuple3 and tuple4

905f33b

Correct intrinsics in the comment

4b011ab

fanyang-mono requested a review from kunalspathak February 7, 2024 22:39

kunalspathak reviewed Feb 8, 2024

View reviewed changes

src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs Show resolved Hide resolved

fanyang-mono added 3 commits February 8, 2024 12:07

Use ld1

2bbe476

Enable tests for mono

7edc0aa

Intrinsify LoadVector*x*AndUnzip

b543e0e

fanyang-mono marked this pull request as ready for review February 8, 2024 17:44

fanyang-mono requested review from lambdageek and vargaz as code owners February 8, 2024 17:44

fanyang-mono requested a review from SamMonoRT as a code owner February 8, 2024 17:44

fanyang-mono changed the title [mono] Intrinsify LoadVector64x* and LoadVector128x* [mono] Intrinsify LoadVector64x*, LoadVector128x*, LoadVector64x*AndUnzip and LoadVector128x*AndUnzip Feb 8, 2024

kunalspathak suggested changes Feb 8, 2024

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Feb 8, 2024

fanyang-mono added 3 commits February 8, 2024 14:39

Uncomment for LoadVector128x*AndUnzip

1e43ecf

Uncomment for LoadVector128x*AndUnzip 2

c77dc71

Intrinsify LoadAndReplicateToVector*x*

4c637b7

fanyang-mono changed the title ~~[mono] Intrinsify LoadVector64x*, LoadVector128x*, LoadVector64x*AndUnzip and LoadVector128x*AndUnzip~~ [mono] Intrinsify multiple LoadVector API's Feb 8, 2024

Merge remote-tracking branch 'origin/main' into intrinsify_loadVector

0018853

Update code after merge with main branch

b707469

kunalspathak approved these changes Feb 12, 2024

View reviewed changes

src/mono/mono/mini/simd-intrinsics.c Show resolved Hide resolved

src/mono/mono/mini/mini-llvm.c Show resolved Hide resolved

src/tests/JIT/HardwareIntrinsics/Arm/Shared/LoadVectorx2Test.template Show resolved Hide resolved

vargaz approved these changes Feb 12, 2024

View reviewed changes

vargaz reviewed Feb 12, 2024

View reviewed changes

src/mono/mono/mini/mini-llvm.c Show resolved Hide resolved

fanyang-mono merged commit 8e2dcfa into dotnet:main Feb 12, 2024

github-actions bot locked and limited conversation to collaborators Mar 18, 2024

Conversation

fanyang-mono commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 7, 2024

Uh oh!

ghost commented Feb 7, 2024

Uh oh!

fanyang-mono commented Feb 7, 2024

Uh oh!

kunalspathak commented Feb 8, 2024

Uh oh!

Uh oh!

fanyang-mono commented Feb 8, 2024

Uh oh!

kunalspathak commented Feb 8, 2024

Uh oh!

Uh oh!

Uh oh!

kunalspathak Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

fanyang-mono Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

kunalspathak Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

fanyang-mono Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

steveisok commented Feb 9, 2024

Uh oh!

fanyang-mono commented Feb 9, 2024

Uh oh!

fanyang-mono commented Feb 9, 2024

Uh oh!

kunalspathak commented Feb 9, 2024

Uh oh!

fanyang-mono commented Feb 9, 2024

Uh oh!

fanyang-mono commented Feb 12, 2024

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fanyang-mono commented Feb 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fanyang-mono commented Feb 7, 2024 •

edited

Loading