Skip to content
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 lots more TensorPrimitives operations #97192

Merged
merged 15 commits into from
Jan 22, 2024

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Jan 19, 2024

All are functional and tested, some are vectorized, others still need to be (subsequent to this PR, tracked by #97193).

Fixes #96451 (still needs final API review)
Fixes #94553 (handles the IndexOfXx methods that were missing)
Fixes #97190
Contributes to #93286

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub stephentoub changed the title Add lots more TensorPrimitive operations Add lots more TensorPrimitives operations Jan 19, 2024
@ghost ghost assigned stephentoub Jan 19, 2024
@ghost
Copy link

ghost commented Jan 19, 2024

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

Issue Details

All are functional and tested, some are vectorized, others still need to be (subsequent to this PR).

Fixes #96451 (still needs final API review)
Fixes #94553 (handles the IndexOfXx methods that were missing)
Fixes #97190

Author: stephentoub
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@stephentoub
Copy link
Member Author

@tannergooding, in addition to normal review stuff, I'd appreciate it if you could triple-check:

  • These are the right overloads to be adding.
  • The generic constraints on each are correct.
  • The method names are what's desired (e.g. I wasn't sure about the MultiplyAddEstimate name, since we call it FusedMultiplyAdd on the interface)

// bool useResult = equal && ((IsNegative(result) == IsNegative(current)) ? (resultIndex < currentIndex) : IsNegative(result));
Vector128<T> resultNegative = IsNegative(result);
Vector128<T> sameSign = Vector128.Equals(resultNegative.AsInt32(), IsNegative(current).AsInt32()).As<int, T>();
useResult |= equalMask & Vector128.ConditionalSelect(sameSign, lessThanIndexMask, resultNegative);
Copy link
Member

Choose a reason for hiding this comment

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

nit: These can be ElementwiseSelect since we know sameSign is AllBitsSet or Zero per-element

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed. Separately, we can go through and replace all ConditionalSelects to be ElementWiseSelect as long as the mask is known to be AllBitsSet or Zero, in particular if it's the result of some comparison operation like Equal, LessThan, GreaterThan, etc., right?

Copy link
Member Author

Choose a reason for hiding this comment

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

(Should we expose ElementWiseSelect on the VectorXX types if it's actually beneficial to use?)

Copy link
Member Author

Choose a reason for hiding this comment

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

(And/or make the JIT recognize some cases where it knows the bit pattern is applicable and do the better thing when possible.)

@stephentoub
Copy link
Member Author

Failure in the "runtime (Build browser-wasm linux Release LibraryTests_EAT)" leg is ##[error] ,##[warning]Received request to deprovision: The request was cancelled by the remote provider.

@stephentoub stephentoub merged commit 8accd80 into dotnet:main Jan 22, 2024
108 of 111 checks passed
@stephentoub stephentoub deleted the allthetensorapis branch January 22, 2024 02:51
@MichalStrehovsky
Copy link
Member

We're seeing failures in main after this merged, see #97087:

Looks like this PR didn't run the legs where this is failing for the Arcade update.

        /_/src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs(215,0): at System.Numerics.Tensors.Tests.GenericFloatingPointNumberTensorPrimitivesTests`1.SpanDestinationFunctions_SpecialValues(SpanDestinationDelegate tensorPrimitivesMethod, Func`2 expectedMethod)
           at InvokeStub_GenericFloatingPointNumberTensorPrimitivesTests`1.SpanDestinationFunctions_SpecialValues(Object, Span`1)
           at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Numerics.Tensors.Tests.DoubleGenericTensorPrimitives.SpanDestinationFunctions_ValueRange(tensorPrimitivesMethod: SpanDestinationDelegate { Method = Void ReciprocalEstimate[Double](System.ReadOnlySpan`1[System.Double], System.Span`1[System.Double]), Target = null }, expectedMethod: Func`2 { Method = Double ReciprocalEstimate(Double), Target = null }) [FAIL]
      Assert.All() Failure: 33 out of 201 items in the collection did not pass.
      [27]:  Item:  Tuple (4, -19)
             Error: Assert.Equal() Failure: Values differ
                    Expected: -0.052490234375
                    Actual:   -0.052631578947368418
      [28]:  Item:  Tuple (4, -16)
             Error: Assert.Equal() Failure: Values differ
                    Expected: -0.0623779296875
                    Actual:   -0.0625

akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Jan 22, 2024
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Add more TensorPrimitive operations

All are functional and tested, some are vectorized, others still need to be.

* Re-fix IndexOfXx operations

* Rename MultiplyAddEstimate to FusedMultiplyAdd

* Remove some NotSupportedException throws

* Simplify CopySignOperator

* Parameter renames

* Add scalar overloads of Atan2 and Atan2Pi

* Add CopySign scalar overload

* Add Ieee754Remainder scalar overloads

* Add Lerp scalar overloads

* Add Pow scalar overload

* Consolidate inverted operators

* Add missing Max/Min scalar overloads

* Use ElementWiseSelect
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.