-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add a number of additional APIs to the various SIMD accelerated vector types #111179
Conversation
Note regarding the
|
Note regarding the
|
c44246f
to
d9bd816
Compare
CC. @dotnet/jit-contrib there's some JIT side changes for review/approval here The summary of changes is:
I added support for the new |
@jkurdek could you please review the mono parts? Do we need to update other codegens for this as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this change doesn't implement the new APIs on mono side, it just reverts to managed. @tannergooding do you plan to bring those in to mono in this PR?
src/mono/mono/mini/simd-intrinsics.c
Outdated
@@ -6525,6 +6562,10 @@ emit_intrinsics (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSignature *fsi | |||
MonoInst* | |||
mono_emit_simd_intrinsics (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSignature *fsig, MonoInst **args) | |||
{ | |||
if (!has_intrinsic_cattr (cmethod)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the impact of this is going to be. if we had some intrinsics for methods which weren't marked [Intrinsic]
before now they gonna experience slow downs. Have you benchmarked this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it match the behavior that RyuJiT has. Anything that isn’t marked Intrinisic is not intended to be handled as such and is likely a Mono bug
If, after merge, there are regressions then we should look at those cases individually and determine if the attribute should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mono may have had intrinsics for a few non-Intrinsic methods previously for performance reasons, but I think it's valid to do this and see how it shakes out as long as we pay attention to the benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mono may have had intrinsics for a few non-Intrinsic methods previously for performance reasons
👍 We should definitely make sure such cases are known and mark them as [Intrinsic]
in those scenarios instead. It massively simplifies things and should also improve Mono throughput, as we won't be doing unnecessary work for every single API anymore.
If it's truly unique to Mono, then we can even do the following; however I expect most places that Mono handles should also be handled on RyuJIT or at least get the inlining profitability boost from the attribute:
#if MONO
[Intrinsic]
#endif
Assuming this PR is merged before the weekend, we should catch any such regressions next Tuesday and be able to trivially root cause the smaller number of samples so we can know what should be attributed.
-- Notably this is being done in this PR for correctness reasons; there are cases where Mono has "bad assumptions" about signatures and where it was trying to treat an API named X
as an intrinsic for X
, even when the signature is a different overload all together and was designed to mean something different. This was the case for some APIs like Vector.Store
which exposes extension methods for Vector<T>
and also Vector2/3/4
as an example.
It implements a few, but not all of them. I plan on finishing up the other new For the other APIs, they would be handled at the same time as support is added to RyuJIT, which as per the top post is not being done yet. |
src/libraries/System.Private.CoreLib/src/System/Numerics/Vector2.Extensions.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Numerics/Vector2.Extensions.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128.cs
Show resolved
Hide resolved
Happy to see a lot of these additions, should cut down on the amount of per-architecture code necessary to solve problems. |
29bf295
to
a242714
Compare
…astIndexOf methods is correct
a242714
to
fa36e8f
Compare
@kg, could you sign off on the Mono side changes? As per my comment above, I plan on putting up a follow up PR to add the necessary helper APIs and start using them on the new intrinsics that weren't completed in this PR (I did get many of the scenarios handled in this PR, however). |
I'll review them again ASAP. I'm out sick right now, so it might be faster to ask someone else. |
Hope you get well soon! @jkurdek would you be able to finish the review instead? |
Sure, I will look into that tommorow morning (European time). It slipped my mind today, sorry! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mono side looks ok for me. Great work!
This resolves #103845 and #98055