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

Ensure the System.Runtime.Intrinsics ref assembly doesn't include incomplete APIs #26365

Closed
tannergooding opened this issue Jun 2, 2018 · 16 comments

Comments

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Jun 2, 2018

This is just a tracking issue to ensure that we don't forget to trim the System.Runtime.Intrinsics ref assembly before the next release.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jun 2, 2018

@saucecontrol
Copy link
Member

@saucecontrol saucecontrol commented Jun 2, 2018

Are the intrinsics coming back for 2.2? I was surprised that the System.Runtime.Intrinsics.Experimental Nuget package was unlisted following the 2.1 release, and it looks like most of the other intrinsics-related issues are tagged with the 3.0 milestone.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jun 2, 2018

@saucecontrol, "if" a new version of the package is published for 2.2 (even on MyGet) we need to ensure that the unimplemented APIs aren't part of the surface area, as calling them will currently crash the runtime 😄

This allows "partially implemented" ISAs to still expose the APIs that have been implemented (for experimental use) without having to hide the entire ISA

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jun 2, 2018

(actually, I think the crashing may have been fixed, and it will just throw a PNSE now)

@voinokin
Copy link

@voinokin voinokin commented Jun 2, 2018

@tannergooding
(Can't find where else and who to ask): Are there any plans on publishing updated S.R.I. again? I mean - I 'm developing high-perf utility which relies heavily on this package, but support for some intrinsics is still not there. The absence of some of them I can workaround, but with performance hit. So I wonder what and when to expect, eh? :-) Thanks.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jun 2, 2018

@eerhardt could probably provide more details.

As a guess, I would imagine it wa unlisted because experimental packages aren't supposed to be published to Nuget (only places like Myget).

I believe the packages are still being uploaded to Myget on a ~nightly basis (I'll check later today and report back, out and about right now)

@saucecontrol
Copy link
Member

@saucecontrol saucecontrol commented Jun 2, 2018

Ah, ok. I was kind of looking for the same info as @voinokin. I'd like to be able to plan around the availability of the Intrinsics support, but I haven't seen anything official as far as release timeline. I thought we'd be able to use the portions that were present in 2.1 RC1 with the caveat that the API was 'experimental', but then even the crashing bugs in the JIT that were reported, fixed, and merged between RC1 and RTW didn't make the release. And then the package was pulled even though partial support is still present in the JIT.

Is there an official plan you can share on that, or is this all up in the air still?

@eerhardt
Copy link
Member

@eerhardt eerhardt commented Jun 4, 2018

Posting my reply from https://github.com/dotnet/corefx/issues/27486#issuecomment-394405562

Has this moved elsewhere, and/or is there any info on the current situation with the hardware intrinsics support?

I found it on MyGet.

Since the package only contains experimental/pre-release APIs, it wasn't intended to be published on nuget.org. It will never be a stable package since when the APIs are considered stable they will be contained in Microsoft.NETCore.App. We accidentally pushed the package to nuget.org as part of our pre-releases of .NET Core 2.1 and didn't notice it until recently.

@airbreather is correct that the official place to get this package is on our myget feed. The 4.5.0-rtm package is the correct version to use with the RTM version of .NET Core 2.1. Don't use the 4.6.x-xxx versions with .NET Core 2.1 because they are not compatible.


Is there an official plan you can share on that, or is this all up in the air still?

We don't have a concrete timeline on when the APIs will be "stable". It depends on a few factors:

  1. Getting the current proposals for API changes reviewed/approved.
  2. How much feedback (both positive and negative) we get on the APIs.
  3. How much other "churn" is happening in the APIs.
    1. example: are we getting a lot of proposals to change the APIs? or are the proposals settling down, and we are confident the APIs will be able to be maintained long-term.
  4. An available .NET Core runtime release that it can ship in.

We will share more when we know more 😄.

@saucecontrol
Copy link
Member

@saucecontrol saucecontrol commented Jun 4, 2018

Thanks @eerhardt, that's really helpful.

Is it correct to assume that the existence of this issue means that until closer to 2.2 release some of the APIs present in the 4.6.0-preview* packages are not yet implemented in the JIT and will either crash or throw PNSE?

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jun 4, 2018

@saucecontrol, that is correct.

You can look at https://github.com/dotnet/coreclr/blob/master/src/jit/hwintrinsicxarch.cpp#L376 to find out which ISAs are fully implemented, which are partially implemented, and which are not implemented at all.

Between 2.1 RTM and the current master, only the FMA intrinsics have been moved (from not implemented to fully implemented).

For the partially implemented, you can look at https://github.com/dotnet/coreclr/blob/master/src/jit/hwintrinsiclistxarch.h to determine which intrinsics are available for a given ISA (look at the function name field).

There have been no additional partially implemented intrinsics since 2.1 RTM.

@saucecontrol
Copy link
Member

@saucecontrol saucecontrol commented Jun 4, 2018

Excellent, thanks much @tannergooding. I'm excited to see how the FMA stuff works out with image convolution. I'll leave you alone now so you can work on the rest ;)

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Jul 17, 2018

Tanner this should be set to milestone=3.0 right? 2.2 is based off release/2.1 for us.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 17, 2018

@danmosemsft, that depends on if we are wanting to ship another preview in 2.2, if yes then this is important. Otherwise, this can probably be closed since the feature is targeting 3.0

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Jul 17, 2018

We do not plan to ship another preview in 2.2 unless it's an inevitable consequence of shipping 2.2 which I assume it is'nt.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 17, 2018

Sounds good. I'll close then.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants