Skip to content

ISimdVector vector intrinsics fixes #113288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Mar 13, 2025
Merged

ISimdVector vector intrinsics fixes #113288

merged 13 commits into from
Mar 13, 2025

Conversation

lewing
Copy link
Member

@lewing lewing commented Mar 8, 2025 •
edited
Loading

#104983 added filtering for explicit ISimdVector implementations. It appears it used the wrong namespace for System.Numerics.Vector<T> when removing the expanded interface and types from the method name. Fix the explicit interface implementation filtering and call SN Vector interpreter intrinsics.

This was referenced Mar 8, 2025
Open
Closed
@lewing lewing force-pushed the test-sn-vector-interp branch from 1e58c80 to 13d6a60 Compare March 8, 2025 05:03
@lewing lewing force-pushed the test-sn-vector-interp branch from 13d6a60 to d7bffcc Compare March 8, 2025 05:05
@lewing lewing changed the title [wasm] testing interp vector intrins [mono] vector intrinsics fixes Mar 8, 2025
This was referenced Mar 8, 2025
Open
Open
Open
@lewing lewing force-pushed the test-sn-vector-interp branch from ebcc1fb to 4f79607 Compare March 9, 2025 23:10
This was referenced Mar 10, 2025
Open
Open
Open
Open
Open
@lewing lewing force-pushed the test-sn-vector-interp branch 3 times, most recently from 934ec82 to b6f4172 Compare March 11, 2025 16:42
@lewing lewing force-pushed the test-sn-vector-interp branch from b6f4172 to b3560ab Compare March 11, 2025 18:03
@lewing lewing marked this pull request as ready for review March 11, 2025 22:11
@Copilot Copilot AI review requested due to automatic review settings March 11, 2025 22:11
@lewing lewing force-pushed the test-sn-vector-interp branch from 418452e to ddb2b8f Compare March 12, 2025 18:38
@lewing
Copy link
Member Author

lewing commented Mar 12, 2025

@tannergooding I assume this was the intent of the original code? if so I'm happy to do the same change in jitimporter/CorInfoImpl.cs?

done

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

@lewing lewing requested a review from EgorBo March 12, 2025 19:16
@lewing lewing force-pushed the test-sn-vector-interp branch from 04516ad to 35bfdea Compare March 12, 2025 19:24
@EgorBo
Copy link
Member

EgorBo commented Mar 12, 2025

@MihuBot

@MihuBot MihuBot mentioned this pull request Mar 12, 2025
Open
@lewing lewing force-pushed the test-sn-vector-interp branch from 750fdc6 to f318510 Compare March 12, 2025 20:11
@jeffhandley jeffhandley requested a review from Copilot March 12, 2025 23:38
Copilot
Copilot AI reviewed Mar 12, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the filtering for explicit ISimdVector implementations by addressing the incorrect namespace used for the Vector intrinsics. Key changes include adjusting the string slicing for processing method names and adding a new branch to handle the Runtime.Intrinsics.Vector namespace.

Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:4442

  • Consider using an 'else if' here if the 'Numerics.Vector' branch is mutually exclusive, to prevent potential double processing of the method name.
if (partialMethodName.StartsWith("Runtime.Intrinsics.Vector"))

jeffhandley
jeffhandley approved these changes Mar 12, 2025
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Looks good to me and I agree with this capturing the original intent, but we should get signoff from a jit expert.

EgorBo
EgorBo approved these changes Mar 12, 2025
View reviewed changes
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM. It's a bit unfortunate that we use magic constants here, any C++ compiler should be able to constant fold strlen("const"), but it's out of the scope of this PR

lateralusX
lateralusX reviewed Mar 13, 2025
View reviewed changes
src/mono/mono/mini/simd-intrinsics.c
(strncmp(cmethod_name + 70, "256<T>,T>.", 10) == 0) ||
(strncmp(cmethod_name + 70, "512<T>,T>.", 10) == 0)) {
cmethod_name += 80;
if (strncmp(cmethod_name, "System.Runtime.Intrinsics.ISimdVector<System.", 45) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we duplicate this code block 3 times, maybe put it into some shared function?

radekdoulik
radekdoulik approved these changes Mar 13, 2025
View reviewed changes
Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

Besides the code duplication it LGTM.

@lewing lewing merged commit ee74dc9 into dotnet:main Mar 13, 2025
173 of 177 checks passed
@lewing lewing deleted the test-sn-vector-interp branch March 13, 2025 14:59
tannergooding
tannergooding reviewed Mar 19, 2025
View reviewed changes
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.

LGTM

(Sorry for the delay, was out last week)

@lewing lewing mentioned this pull request Mar 20, 2025
Open
@github-actions github-actions bot locked and limited conversation to collaborators Apr 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tannergooding tannergooding tannergooding left review comments

@lateralusX lateralusX lateralusX left review comments

Copilot code review Copilot Copilot left review comments

@EgorBo EgorBo EgorBo approved these changes

@jeffhandley jeffhandley jeffhandley approved these changes

@radekdoulik radekdoulik radekdoulik approved these changes

@steveisok steveisok Awaiting requested review from steveisok steveisok is a code owner

@vitek-karas vitek-karas Awaiting requested review from vitek-karas vitek-karas is a code owner

@BrzVlad BrzVlad Awaiting requested review from BrzVlad BrzVlad is a code owner

@kotlarmilos kotlarmilos Awaiting requested review from kotlarmilos kotlarmilos is a code owner

@kg kg Awaiting requested review from kg

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees

@lewing lewing

Labels
area-Codegen-Interpreter-mono area-Codegen-Intrinsics-mono area-System.Runtime.Intrinsics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lewing @vitek-karas @EgorBo @jeffhandley @radekdoulik @tannergooding @lateralusX