Skip to content

Vectorizes IndexOfMin/Max/Magnitude #93469

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
michaelgsharp merged 20 commits into from
Oct 20, 2023
Merged

Vectorizes IndexOfMin/Max/Magnitude #93469

michaelgsharp merged 20 commits into from
Oct 20, 2023

Conversation

@michaelgsharp
Copy link
Contributor

@michaelgsharp michaelgsharp commented Oct 13, 2023

Adds a vectorized implementation of IndexOfMin, IndexOfMax, IndexOfMaxMagnitude, IndexOfMinMagnitude.

@ghost ghost assigned michaelgsharp Oct 13, 2023
@ghost ghost added the area-System.Numerics label Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

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

Issue Details

Adds a vectorized implementation of IndexOfMin, IndexOfMax, IndexOfMaxMagnitude, IndexOfMinMagnitude.

Author: michaelgsharp
Assignees: michaelgsharp
Labels:

area-System.Numerics
Milestone: -

@michaelgsharp
Copy link
Contributor Author

michaelgsharp commented Oct 13, 2023

I'm still finishing up vectorizing the NetStandard version so its not included here but I wanted to get it up for review. Should have the NetStandard done here shortly. When its done the ifdefs in TensorPrimitives.cs will go away, so don't worry about those.

Also, not sure if the .sln change should be there or not? Anyone know?

stephentoub
stephentoub reviewed Oct 13, 2023
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Oct 13, 2023
Open
5 tasks
@michaelgsharp michaelgsharp marked this pull request as ready for review October 16, 2023 18:05
@ericstj
Copy link
Member

ericstj commented Oct 16, 2023 •
edited
Loading

Failures here are test failures, the MathFMaxMagnitude uses IndexOfMaxMagnitude which is changed in this PR.
https://github.com/dotnet/runtime/blob/45ed0ab41c138ad1683b333b5b937b8240f3e4d8/src/libraries/System.Numerics.Tensors/tests/TensorPrimitivesTests.cs#L1476C70-L1476C70
I suspect this PR broke those tests. Probably you don't have those tests locally as Stephen recently added them.

ericstj
ericstj reviewed Oct 17, 2023
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Oct 19, 2023
Open
stephentoub
stephentoub requested changes Oct 19, 2023
View reviewed changes
@ghost 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 Oct 19, 2023
stephentoub
stephentoub reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but two issues to be addressed.

@build-analysis build-analysis bot mentioned this pull request Oct 20, 2023
Closed
@ericstj ericstj self-requested a review October 20, 2023 15:07
@michaelgsharp michaelgsharp merged commit f3f0af8 into dotnet:main Oct 20, 2023
@michaelgsharp michaelgsharp deleted the indexof branch October 20, 2023 17:19
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@tannergooding tannergooding Awaiting requested review from tannergooding

@ericstj ericstj Awaiting requested review from ericstj

Assignees

@michaelgsharp michaelgsharp

Labels

area-System.Numerics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@michaelgsharp @ericstj @stephentoub @tannergooding