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

S.R.Intrinsics.X86 for .NET Standard? #23689

Closed
ektrah opened this issue Sep 29, 2017 · 22 comments
Closed

S.R.Intrinsics.X86 for .NET Standard? #23689

ektrah opened this issue Sep 29, 2017 · 22 comments
Labels
area-System.Runtime.CompilerServices enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@ektrah
Copy link
Member

ektrah commented Sep 29, 2017

I've been loosely following the S.R.Intrinsics.X86 feature; apologies if this has been discussed before.

As far as I know, S.R.Intrinsics.X86 requires support both from the hardware and from the CLR. Currently, the IsSupported properties indicate whether hardware support is present. This allows users to write code like this:

if (Sse2.IsSupported)
{
    // fast path
}
else
{
    // slow path
}

The S.R.Intrinsics.X86 package is currently only available for netcoreapp2.1. This means if users want to target .NET Standard, they have to compile two versions of their library; one for .NET Core 2.1 and one for .NET Standard:

#if NETCOREAPP2_1
if (Sse2.IsSupported)
{
    // fast path
}
else
{
#endif
    // slow path
#if NETCOREAPP2_1
}
#endif

Could the package be changed to target .NET Standard and the IsSupported properties be changed to indicate whether both hardware support and CLR support is available?

Then users could simply target .NET Standard and any environment that has the right hardware and CLR would automatically take the fast path, without users having to compile their library individually for every CLR that supports the feature now or in the future.

@jkotas
Copy link
Member

jkotas commented Sep 30, 2017

This makes sense.
cc @fiigii @mellinoe

@fiigii
Copy link
Contributor

fiigii commented Sep 30, 2017

That would be great if Intel hardware intrinsic could target to .NET Standard. If this is allowed, I would change the implementation of IsSupported.

@fiigii
Copy link
Contributor

fiigii commented Oct 2, 2017

@mellinoe ping?

@mellinoe
Copy link
Contributor

mellinoe commented Oct 2, 2017

We could probably add this fairly easily. Given that it's already a capability-based API, it should work fairly naturally. By that, I mean you already need to check if an operation is supported before calling it, so it sort of naturally extends to additional targets which don't support the operation. We will need to add a large number of "dummy" calls into the codebase here which all throw PlatformNotSupportedException and/or return false. Other than that it should be simple.

@fiigii
Copy link
Contributor

fiigii commented Oct 2, 2017

@mellinoe Thanks, I just wanted to make sure it is allowed to target S.R.Intrinsics.X86 for .NET Standard.

We will need to add a large number of "dummy" calls into the codebase here which all throw PlatformNotSupportedException and/or return false.

This is already done by dotnet/coreclr#14164 in mscorlib. So I should do this in CoreFX as well?

@jkotas
Copy link
Member

jkotas commented Oct 2, 2017

This is already done by dotnet/coreclr#14164 in mscorlib. So I should do this in CoreFX as well?

Yes.

@fiigii
Copy link
Contributor

fiigii commented Oct 2, 2017

Thank you, will do.

@weshaggard
Copy link
Member

@fiigii @jkotas @mellinoe do we have any plans to ship this library in the shared framework or will it always be an OOB library? I ask because if it ships in the shared framework it will put restrictions on evolving the API OOB later if we start targeting this to netstandard.

@mellinoe
Copy link
Contributor

mellinoe commented Oct 4, 2017

I think there is a good chance that it will ship in the shared framework at some point.

@jkotas
Copy link
Member

jkotas commented Oct 4, 2017

This library is tightly coupled with the runtime, so it cannot be OOB.

@weshaggard
Copy link
Member

This library is tightly coupled with the runtime, so it cannot be OOB.

If that is the case I would suggest it be added to the framework and not have a nuget package at all, and thus not have a netstandard implementation either.

@jkotas
Copy link
Member

jkotas commented Oct 5, 2017

Given the most recent discussions about the complexity that come with the partial OOBs - I agree.

@fiigii
Copy link
Contributor

fiigii commented Oct 5, 2017

So the conclusion is that we can stop dotnet/corefx#24415. Then combine S.R.Intrinsics.X86 into the framework and delete the nuget package. After that, only netcoreapp can access these intrinsics. Is it right?
@jkotas @weshaggard @mellinoe @joperezr

@weshaggard
Copy link
Member

Yes that is a correct summary. Can you handle the removal of the package and adding it to the shared framework?

@fiigii
Copy link
Contributor

fiigii commented Oct 6, 2017

Can you handle the removal of the package and adding it to the shared framework?

Sure, will do. But I have no experience on the shared framework, could you point an example to me? @weshaggard @jkotas

@weshaggard
Copy link
Member

I believe the only thing that is needed is to set IsNETCoreApp=true in the dir.props like https://github.com/dotnet/corefx/blob/master/src/System.Runtime/dir.props#L7, and remove the pkg directory completely.

@fiigii
Copy link
Contributor

fiigii commented Oct 6, 2017

@weshaggard Thanks! Will submit a PR.

@devsko
Copy link
Contributor

devsko commented Nov 1, 2017

Does this mean the intrinsics will be only available in netcorapp? It would be great to use them also in WPF or UWP.
(Sorry, I am still confused about what this standard/shared/common stuff really means)

@weshaggard
Copy link
Member

@devsko that is correct. This will only be available initially for netcoreapp, and then we will decide if we should port it to .NET Framework.

@jkotas
Copy link
Member

jkotas commented May 25, 2018

Question from @colgreen

I maintain a code library/nuget that would benefit from using these hardware intrinsics, but it currently targets .NET Standard to provide good portability.

Ideally I'd like to continue to offer portability, but also improved performance if the runtime platform/environment provides these intrinsics. Right now it seems my choice is either speed or portability, but not both - is this likely to change in the future?

@jkotas
Copy link
Member

jkotas commented May 25, 2018

If you want both portability and speed for platform/environment provided by these intrinsics, you would need to build your library twice: once for netstandard and once for netcoreapp, and include both in your NuGet package. It is pretty common for top tier nuget packages to include multiple builds optimized for different targets. For example, https://www.nuget.org/packages/Newtonsoft.Json/11.0.2 contains 9 different builds.

@colgreen
Copy link
Contributor

@jkotas ok understood. Thanks for the response.

I have a general purpose library where this makes sense, and another library (that consumes the other one) that is related to machine learning where these h/w intrinsics may be very useful, and thus I'm tempted to provide a dotnet core version only. The main downside right now is that ultimately there's a winforms app at the top of the stack (i.e. that provides a GUI over the ML lib), which must therefore target .NET Framework and thus MS Windows. Hence to-date dotnet standard has been a handy solution for me for the non-GUI code lower down the stack.

Ideally there would be a dotnet core UI framework, but I also read that dotnet core 3 may have support for winforms (for MS Windows only), which is a reasonable compromise, i.e. you get the option of a UI app on Windows (and the h/w intrinsics), but only a console app in non-Windows.

Thanks again.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.CompilerServices enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants