Report SIMD intrinsics to RyuJIT by MichalStrehovsky · Pull Request #3678 · dotnet/corert

MichalStrehovsky · 2017-05-23T00:26:13Z

Implement isInSIMDModule
Implement appendClassName
Implement a type name formatter that roughly approximates
typestring.cpp in the CLR. Should be enough for the purposes of
recognizing SIMD intrinsics. Hopefully, we won't ever have to be
complete. This is the formatter used by reflection - the rules are
complex and long.

* Implement `isInSIMDModule` * Implement `appendClassName` * Implement a type name formatter that roughly approximates typestring.cpp in the CLR. Should be enough for the purposes of recognizing SIMD intrinsics. Hopefully, we won't ever have to be complete. This is the formatter used by reflection - the rules are complex and long.

jkotas · 2017-05-23T05:26:57Z

Do we also need to adjust the size of Vector<T> accordingly, or are we getting lucky?

(Look for https://github.com/dotnet/coreclr/search?q=getMaxIntrinsicSIMDVectorLength in CoreCLR.)

MichalStrehovsky · 2017-05-31T18:13:46Z

Do we also need to adjust the size of Vector<T> accordingly, or are we getting lucky?

Without AVX, the size in IL (16) matches the size used by RyuJIT. But I made the changes to also support AVX in the future (once we make RyuJIT stop conditioning it on conditions that might make sense for a JIT, but don't make sense for an AOT compiler).

Instead of CompilerTypeSystemContext calling back into RyuJIT to determine sizes of things, I'm guarding this with asserts. I don't think we should have a RyuJitTypeSystemContext or a set of weird callbacks between Compilation and CompilerTypeSystemContext. The native vector sizes should be specified by the driver, not by the codegen in an AOT compiler.

I don't particularly like how ingrained Vector<T> became into the type system, but I guess there's no way around it. We might also want to make it a WellKnownType, actually, to make checking for it cheaper...

Cc @davidwrighton

MichalStrehovsky · 2017-05-31T18:15:26Z

src/JitInterface/src/CorInfoImpl.cs

+        {
+            // We support enough of this to make SIMD work, but not much else.
+
+            // TODO: figure out why the marshalling for fFullInst and fAssembly is wrong.


This kinda looks like a bug in marshalling. I couldn't figure out what's wrong with the signature. Suggestions welcome.

Should be UnmanagedType.Bool from what I can see.

Found the culprit: the generated jitinterface.h we use as the wrapper declares this method as int (__stdcall * appendClassName)(void * thisHandle, CorInfoException** ppException, wchar_t** ppBuf, int* pnBufLen, void* cls, bool fNamespace, bool fFullInst, bool fAssembly);. There's a bool/BOOL mismatch.

We need to either fix the generated wrapper code, or our mashalling annotations. Probably former rather than the latter.

Either way, out of scope of this pull request.

MichalStrehovsky · 2017-05-31T18:16:54Z

src/Native/jitinterface/jitwrapper.cpp

+    Jit * pJit,
+    CORJIT_FLAGS flags)
+{
+    // BUGBUG: the signature on the Jit side is actually "pass by value", but for reasons that escape me,


Again, couldn't figure out why the ABI doesn't match. The flags that come from the managed side look alright in the debugger, but become broken on the RyuJIT side. Of course it can't be checked in like this, but I already spent too much time on this and need a fresh pair of eyes to have a look.

This may have to do with different calling convention for Plain-Old-Datatype structs vs. C++ structs. Does it change if you add constructor to CORJIT_FLAGS ?

Adding a copy constructor did the trick. Thanks!

MichalStrehovsky · 2017-05-31T18:18:06Z

src/Common/src/TypeSystem/CodeGen/TargetDetails.CodeGen.cs

+
+namespace Internal.TypeSystem
+{
+    // Extension to TargetDetails related to code generation


We might want to kick things like the Abi to this flavor of TargetDetails too.

MichalStrehovsky · 2017-05-31T19:03:03Z

src/Common/src/TypeSystem/CodeGen/TargetDetails.CodeGen.cs

+
+        public MaximumSimdVectorLength MaximumSimdVectorLength
+        {
+            get;


Now that I had a chance to think about this a little on the way to get lunch: would we just want to introduce this as a TargetArchitecture flavor? E.g. TargetArchitecture.x64, TargetArchitecture.x64_Avx, with a IsX64 convenience property on TargetDetails? (Similar for other arches, in the future.) Yes, it feels weird to put instruction set details on a type system object, but as we see here, this does affect type layout, and it is a type system concern.

I don't really like that idea. Its very much readable to have a switch case arrangement around use of target architecture, and while it doesn't cover all cases, its pretty good. Forcing the use of a series of helper functions is a bit of a kludge.

davidwrighton

Looks basically good, but please fix or at the very least understand the marshaling issues before checking in.

davidwrighton · 2017-05-31T19:43:34Z

src/Common/src/TypeSystem/CodeGen/TargetDetails.CodeGen.cs

+
+        public MaximumSimdVectorLength MaximumSimdVectorLength
+        {
+            get;


I don't really like that idea. Its very much readable to have a switch case arrangement around use of target architecture, and while it doesn't cover all cases, its pretty good. Forcing the use of a series of helper functions is a bit of a kludge.

davidwrighton · 2017-05-31T19:46:21Z

src/Common/src/TypeSystem/CodeGen/TargetDetails.CodeGen.cs

+        None,
+        VectorLength16,
+        VectorLength32,
+    }


I don't like these enum names. What does the 16 or 32 refer to? Bytes? Bits? Elements? In general, I've seen vectors named based on bit length more than on byte length. (For instance, AVX512) I'd prefer something like Vector64Bit

davidwrighton · 2017-05-31T19:52:00Z

src/JitInterface/src/CorInfoImpl.cs

+        {
+            // We support enough of this to make SIMD work, but not much else.
+
+            // TODO: figure out why the marshalling for fFullInst and fAssembly is wrong.


Should be UnmanagedType.Bool from what I can see.

Only stomp over the field size and byte count for maximum CLR compat.

dnfclas added the cla-already-signed label May 23, 2017

MichalStrehovsky added 2 commits May 30, 2017 12:54

Merge branch 'master' into simd

2859e8e

CR feedback

3d4fae8

MichalStrehovsky commented May 31, 2017

View reviewed changes

davidwrighton approved these changes May 31, 2017

View reviewed changes

MichalStrehovsky added 4 commits May 31, 2017 13:42

Fix ABI mismatch

7fac905

Use fallback metadata algorithm

2a52348

Only stomp over the field size and byte count for maximum CLR compat.

Review feedback

c5f1b26

Cleanups

6fc5b2a

MichalStrehovsky merged commit f86c727 into dotnet:master Jun 1, 2017

MichalStrehovsky deleted the simd branch June 1, 2017 04:30

MichalStrehovsky mentioned this pull request Aug 2, 2018

Enable System.Runtime.Intrinsics intrinsics #6173

Open

4 tasks

MichalStrehovsky mentioned this pull request Jan 16, 2019

Enable detection of HW intrinsics #6836

Merged

Conversation

MichalStrehovsky commented May 23, 2017

Uh oh!

jkotas commented May 23, 2017

Uh oh!

MichalStrehovsky commented May 31, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants