- Notifications
You must be signed in to change notification settings - Fork5.2k
Update Enumerable.Min/Max for all IBinaryIntegers#96605
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The implementations are special-casing most of the built-in ones, in order to delegate to the IBinaryInteger-constrained helper, but it was missing Int128, UInt128, and char. ?hese won't be vectorized, but they'll at least use the more streamlined non-vectorized implementations.
ghost commentedJan 8, 2024
Tagging subscribers to this area: @dotnet/area-system-linq Issue DetailsThe implementations are special-casing most of the built-in ones, in order to delegate to the IBinaryInteger-constrained helper, but it was missing Int128, UInt128, and char. ?hese won't be vectorized, but they'll at least use the more streamlined non-vectorized implementations.
|
eiriktsarpalis left a comment
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.
If vectorization isn't available for the new types, what is the primary motivation for making this change? Is it thatIBinaryInteger comparison is generally faster than usingComparer<TDefault>? Or is it simply the fact that we extract the span and avoid allocating an enumerator? If it's the latter case, couldn't this be extracted to the outer method so that it can be applied to all types?
stephentoub commentedJan 8, 2024
Previously a) it was a bit faster and b) IsSupported could ironically only be called with a supported type or else it would throw. But the former was improved in the JIT and the latter was fixed late in 8, so we should be able to make it work with an IComparable-based implementation. The primary benefit is being able to work on the span and avoid the enumerator costs. |
stephentoub commentedJan 8, 2024
Althoughhttps://github.com/dotnet/runtime/pull/96570/files#diff-a7055cca652c7bf1b5af1f4f86111a7949edc40480117bf2fd7651b9338458ca needs to be merged first to remove the constraint from TryGetSpan. |
stephentoub commentedJan 8, 2024
Since this is a small change, I suggest we merge this, and then we can subsequently look at improving Min/Max to use a span for other types. I have another branch where I'm doing so for more operators and I can incorporate it into that. |
stephentoub commentedJan 10, 2024
I gave this a go, and the reliance on |
PRdotnet#96605 added new generic arguments to the unit tests which need tobe preserved in the rd.xml since the test suite is not yet trim-compatible.
PR#96605 added new generic arguments to the unit tests which need tobe preserved in the rd.xml since the test suite is not yet trim-compatible.
PRdotnet#96605 added new generic arguments to the unit tests which need tobe preserved in the rd.xml since the test suite is not yet trim-compatible.
The implementations are special-casing most of the built-in ones, in order to delegate to the IBinaryInteger-constrained helper, but it was missing Int128, UInt128, and char. ?hese won't be vectorized, but they'll at least use the more streamlined non-vectorized implementations.