[mono][interp] Add instrinsics for common Vector128 operations #81782

BrzVlad · 2023-02-07T20:18:13Z

We add intrinsics for Vector128 intrinsics that are actively used within our bcl.

We declare a set of simd method names, the same way we do it with jit, in simd-methods.def. In transform-simd.c we lookup method names in the list of supported intrinsics for Vector128 and Vector128<T>. Once we find a supported instrinsic, we generate code for it, typically a MINT_SIMD_INTRINS_* opcode. In order to avoid adding too many new opcodes to the interpreter, simd intrinsics are grouped by signature. So all simd intrinsics that receive a single argument and return a value, will be called through MINT_SIMD_INTRINS_P_P. This instruction will receive an index to get the intrinsic implementation and calls it indirectly.

Some of the intrinsics are implemented using the standard vector intrinsics, supported by gcc and clang. These do not fully expose the SIMD capabilities, so some intrinsics are implemented naively. This should still be faster than using nonvectorized approach from managed code. In the future we can add better implementation, on platforms where we have low level support. This would both be faster and reduce code size.

Vectorized algorithms that I've tested on interpreter tend to be about 1.5x - 2x faster than their non-vectorized version. On wasm, the jiterpreter handles pretty well non-vectorized algorithms (since they are typically a set of simple instructions that all get compiled to a single trace) and this PR doesn't produce a clear perf improvement. Wasm should get an additional speedup once SIMD support is added into the jiterpreter.

ghost · 2023-02-07T20:18:25Z

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

Issue Details

null

Author:	BrzVlad
Assignees:	BrzVlad
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

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

src/mono/mono/mini/interp/transform-simd.c

src/mono/mono/mini/interp/transform.h

BrzVlad · 2023-02-15T16:34:53Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-15T16:35:22Z

Azure Pipelines successfully started running 1 pipeline(s).

kg · 2023-02-16T02:06:35Z

Don't have a full grip on this PR yet to the point that I could checkmark it, but it seems promising so far. I have a basic jiterpreter implementation for it such that there are no trace aborts due to SIMD opcodes.

lambdageek · 2023-02-16T14:39:17Z

This is a lot of boilerplate that is very similar to what we do for the JIT/AOT. Is there some way we can share more between the execution engines? There's a ton of these SIMD opcodes it would be better if we could generate the code for each execution engine from a single abstract template. Maybe a python script or something...

/cc @jandupej

tannergooding · 2023-02-16T15:29:07Z

Is there some way we can share more between the execution engines?

RyuJIT handles this via a single header file per architecture: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsiclistxarch.h
https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsiclistarm64.h

And a general header that defines the enums + basic structs + basic helper APIs: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsic.h

We have an entry per intrinsic that tracks most of the table drivable information and a few flags to help cover anything that needs special handling or lookups (which happens through well-defined helper methods/etc).

I imagine a very similar setup would work for JitIntrinsics -> LlvmIntrinsics or for JitIntrinsics -> MonoInstructions

Mono being C only somewhat restricts the ability for some of this to be shared between RyuJIT and Mono, but that might be fine overall.

jandupej · 2023-02-16T16:17:29Z

This is a lot of boilerplate that is very similar to what we do for the JIT/AOT. Is there some way we can share more between the execution engines? There's a ton of these SIMD opcodes it would be better if we could generate the code for each execution engine from a single abstract template. Maybe a python script or something...

/cc @jandupej

I agree that would be beneficial. It will take some tuning to get this tabular approach right and applicable to mini, LLVM and interpreter. So my plan is to start small and do it with a single category of operations first (e.g. comparisons). When the final form is ironed out, we will convert existing code to the new form.

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

kg · 2023-02-17T03:38:21Z

Shelving the jiterpreter bits for now, but they're at kg@a38fa17.

BrzVlad · 2023-02-20T09:42:50Z

There are no issues that I'm aware of with this PR. It needs a final approval so we can get it in and get more perf numbers.

We add intrinsics for Vector128 intrinsics that are actively used within our bcl. We declare a set of simd method names, the same way we do it with jit, in `simd-methods.def`. In `transform-simd.c` we lookup method names in the list of supported intrinsics for `Vector128` and `Vector128<T>`. Once we find a supported instrinsic, we generate code for it, typically a `MINT_SIMD_INTRINS_*` opcode. In order to avoid adding too many new opcodes to the interpreter, simd intrinsics are grouped by signature. So all simd intrinsics that receive a single argument and return a value, will be called through `MINT_SIMD_INTRINS_P_P`. This instruction will receive an index to get the intrinsic implementation and calls it indirectly. Some of the intrinsics are implemented using the standard vector intrinsics, supported by gcc and clang. These do not fully expose the SIMD capabilities, so some intrinsics are implemented naively. This should still be faster than using nonvectorized approach from managed code. In the future we can add better implmentation, on platforms where we have low level support. This would both be faster and reduce code size.

These intrinsics are not yet implemented on jiterpreter, making it slighty slower instead.

v128_create receives as an argument every single element of the vector. This method is typically used with constants. For a Vector128<short> this means that creating a constant vector required 8 ldc.i4 and a v128_create. We can instead use a single instruction and embed the vector value in the code stream directly.

It is actually not used in bcl, it is not really vectorized on any platforms and the codegen for the interp implementation is massive and inefficient.

…extension

lambdageek

I think we should disable WASI
Why can't we use __builtin_shuffle(vec1, mask)

lambdageek · 2023-02-23T20:26:31Z

src/mono/mono/mini/interp/interp-internals.h

@@ -101,6 +102,10 @@ typedef enum {

 #define PROFILE_INTERP 0

+#if !HOST_BROWSER && __GNUC__


Suggested change

#if !HOST_BROWSER && __GNUC__

#if !HOST_BROWSER && !HOST_WASI && __GNUC__

The real reason for disabling on browser is not because this PR has some problems with wasm, but because this PR leads to jiterpreter trace aborts since support is not yet added. On WASI there is no jiterpreter support so this PR should still have perf gains.

lambdageek · 2023-02-23T20:43:30Z

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

+
+// Shuffle
+
+#define V128_SHUFFLE(eltype, itype) do {					\


Curious why you have to do this by hand and not with __builtin_shuffle ?

I simply didn't want to make use of any additional builtins for now, to keep it as simple as possible and reduce potential compatibility issues. Will start adding various builtins in future PR's, probably even arch specific ones. I think __builtin_shufflevector has better support and is very powerful, so might be a better candidate.

[mono][interp] Move defines from transform.c

c439344

dotnet-issue-labeler bot added the area-Codegen-Interpreter-mono label Feb 7, 2023

ghost assigned BrzVlad Feb 7, 2023

tannergooding reviewed Feb 7, 2023

View reviewed changes

src/mono/mono/mini/interp/interp-simd.c Outdated Show resolved Hide resolved

tannergooding reviewed Feb 7, 2023

View reviewed changes

src/mono/mono/mini/interp/interp-simd.c Outdated Show resolved Hide resolved

tannergooding reviewed Feb 7, 2023

View reviewed changes

src/mono/mono/mini/interp/interp-simd.c Outdated Show resolved Hide resolved

radekdoulik mentioned this pull request Feb 7, 2023

[wasm] Interpreter SIMD support #81785

Closed

BrzVlad force-pushed the feature-interp-vector128 branch from f105d35 to 519b31b Compare February 9, 2023 09:39

build-analysis bot mentioned this pull request Feb 9, 2023

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

BrzVlad force-pushed the feature-interp-vector128 branch from 349d045 to 96cad8e Compare February 13, 2023 16:08

BrzVlad marked this pull request as ready for review February 14, 2023 06:41

BrzVlad requested review from vargaz, kotlarmilos, lambdageek and SamMonoRT as code owners February 14, 2023 06:41

kotlarmilos reviewed Feb 14, 2023

View reviewed changes

src/mono/mono/mini/interp/interp-simd.c Outdated Show resolved Hide resolved

src/mono/mono/mini/interp/transform-simd.c Outdated Show resolved Hide resolved

src/mono/mono/mini/interp/transform.h Outdated Show resolved Hide resolved

[mono][interp] Move more defines from transform.c

d4eb2aa

BrzVlad force-pushed the feature-interp-vector128 branch from 96cad8e to 3a44372 Compare February 15, 2023 13:52

build-analysis bot mentioned this pull request Feb 15, 2023

Tracking issue for CI build timeouts #76454

Closed

lewing requested a review from kg February 16, 2023 01:57

lambdageek requested a review from jandupej February 16, 2023 14:40

jandupej reviewed Feb 16, 2023

View reviewed changes

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

BrzVlad added 7 commits February 21, 2023 18:03

[mono][interp] Add option to disable simd intrinsics

d42c15c

[mono][interp] Disable simd intrinsics by default on wasm

f69a049

These intrinsics are not yet implemented on jiterpreter, making it slighty slower instead.

[mono][interp] Remove op_Division

8ffac07

It is actually not used in bcl, it is not really vectorized on any platforms and the codegen for the interp implementation is massive and inefficient.

[mono][interp] Don't emit intrinsics for unsupported vector types

629cd94

[mono][interp] Vector extensions used in these intrinsics are a GNUC …

719523d

…extension

BrzVlad force-pushed the feature-interp-vector128 branch from 3a44372 to 719523d Compare February 21, 2023 16:03

This was referenced Feb 21, 2023

Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979

Closed

System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildInvalidSignatureTwice failure #65448

Closed

lambdageek approved these changes Feb 23, 2023

View reviewed changes

BrzVlad merged commit 30283df into dotnet:main Feb 28, 2023

kotlarmilos mentioned this pull request Mar 17, 2023

[Perf] Linux/x64: 178 Regressions on 12/21/2022 11:29:15 AM dotnet/perf-autofiling-issues#11147

Closed

BrzVlad mentioned this pull request Mar 29, 2023

Mono interpreter performance optimizations #47520

Open

24 tasks

ghost locked as resolved and limited conversation to collaborators Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono][interp] Add instrinsics for common Vector128 operations #81782

[mono][interp] Add instrinsics for common Vector128 operations #81782

BrzVlad commented Feb 7, 2023 •

edited

Loading

ghost commented Feb 7, 2023

BrzVlad commented Feb 15, 2023

azure-pipelines bot commented Feb 15, 2023

kg commented Feb 16, 2023

lambdageek commented Feb 16, 2023

tannergooding commented Feb 16, 2023 •

edited

Loading

jandupej commented Feb 16, 2023

kg commented Feb 17, 2023

BrzVlad commented Feb 20, 2023

lambdageek left a comment

lambdageek Feb 23, 2023

BrzVlad Feb 27, 2023

lambdageek Feb 23, 2023

BrzVlad Feb 27, 2023

		@@ -101,6 +102,10 @@ typedef enum {

		#define PROFILE_INTERP 0

		#if !HOST_BROWSER && __GNUC__

	#if !HOST_BROWSER && __GNUC__
	#if !HOST_BROWSER && !HOST_WASI && __GNUC__

[mono][interp] Add instrinsics for common Vector128 operations #81782

[mono][interp] Add instrinsics for common Vector128 operations #81782

Conversation

BrzVlad commented Feb 7, 2023 • edited Loading

ghost commented Feb 7, 2023

BrzVlad commented Feb 15, 2023

azure-pipelines bot commented Feb 15, 2023

kg commented Feb 16, 2023

lambdageek commented Feb 16, 2023

tannergooding commented Feb 16, 2023 • edited Loading

jandupej commented Feb 16, 2023

kg commented Feb 17, 2023

BrzVlad commented Feb 20, 2023

lambdageek left a comment

Choose a reason for hiding this comment

lambdageek Feb 23, 2023

Choose a reason for hiding this comment

BrzVlad Feb 27, 2023

Choose a reason for hiding this comment

lambdageek Feb 23, 2023

Choose a reason for hiding this comment

BrzVlad Feb 27, 2023

Choose a reason for hiding this comment

BrzVlad commented Feb 7, 2023 •

edited

Loading

tannergooding commented Feb 16, 2023 •

edited

Loading