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

Use generic math in System.Numerics.Vectors (#60365) #60905

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

SkiFoD
Copy link
Contributor

@SkiFoD SkiFoD commented Oct 27, 2021

I preserved Sqrt test logic for types that don't inherit IFloatingPoint. T.Sqrt and T.Abs usages fire the CS0119 error, however it doesn't prevent building or tests to pass.

Fixes #60365

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 27, 2021
@ghost
Copy link

ghost commented Oct 27, 2021

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

Issue Details

I preserved Sqrt test logic for types that don't inherit IFloatingPoint. T.Sqrt and T.Abs usages fire the CS0119 error, however it doesn't prevent building or tests to pass.

Author: SkiFoD
Assignees: -
Labels:

area-System.Numerics, community-contribution

Milestone: -

@jkotas
Copy link
Member

jkotas commented Oct 27, 2021

We do not have the support for static virtual methods fully implemented yet. See #49904 and #54063. It is likely the reason why the Mono PR CI legs fails. There are going to be more failures in outer loop test runs.

We can start using the generic math as implementation details of other features only once the static virtual methods are fully implemented.

@jkotas jkotas added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 27, 2021
@tannergooding tannergooding linked an issue Oct 27, 2021 that may be closed by this pull request
@akoeplinger
Copy link
Member

akoeplinger commented Nov 3, 2021

I tried running this on an iOS device with FullAOT and we do indeed run into some generic math AOT limitation there because it tries to JIT, e.g. System.ExecutionEngineException : Attempting to JIT compile method 'int16 System.Numerics.Tests.Util:Add<int16> (int16,int16)' while running in aot-only mode. .

So looks like we can't merge this PR until that is fixed.

@tannergooding
Copy link
Member

Do we have any ETA on when the Mono and Crossgen2 issues will get looked at?

This is going to impact this PR as well as the follow up work I need to do here for .NET 7 and making the feature non-preview.

@akoeplinger
Copy link
Member

@lambdageek any idea?

@tannergooding
Copy link
Member

tannergooding commented Feb 2, 2022

Merged with dotnet/main as per #49904 (comment) from @lambdageek its believed that Mono should be in a state where we can use this in the BCL.

Edit: same with #54063 (comment) from @trylek

@tannergooding tannergooding removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 2, 2022
Copy link
Contributor

@deeprobin deeprobin left a comment

Choose a reason for hiding this comment

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

LGTM

@tannergooding
Copy link
Member

Merged again to pick up the CI fixes for the helix failures.

@tannergooding
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

@lambdageek, looks to be some tvOS failures on the Mono side that will prevent this from merging.

Could you please investigate as this is blocking our ability to work on and merge Generic Math related changes?

@tannergooding tannergooding added the blocked Issue/PR is blocked on something - see comments label Feb 8, 2022
@lambdageek
Copy link
Member

lambdageek commented Feb 8, 2022

tvOS failures look to be the same as #65002

@lambdageek
Copy link
Member

@tannergooding merge/rebase with main to pick up #65126 which should address the tvOS failures

@lambdageek
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

Merging. As with #63546, it looks like the blocking tvOS issue is gone and everything else is passing/looks good.

@tannergooding tannergooding merged commit 58ed181 into dotnet:main Feb 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use generic math in System.Numerics.Vectors test utils
7 participants