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 #30091

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

Comments

Projects
None yet
6 participants
@tannergooding
Member

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

This comment has been minimized.

Member

tannergooding commented Jun 2, 2018

@saucecontrol

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

tannergooding commented Jun 2, 2018

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

@voinokin

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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?

@saucecontrol

This comment has been minimized.

Member

saucecontrol commented Jun 2, 2018

@eerhardt

This comment has been minimized.

Member

eerhardt commented Jun 4, 2018

Posting my reply from #27486 (comment)

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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 ;)

@danmosemsft

This comment has been minimized.

Member

danmosemsft 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

This comment has been minimized.

Member

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

@danmosemsft

This comment has been minimized.

Member

danmosemsft 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

This comment has been minimized.

Member

tannergooding commented Jul 17, 2018

Sounds good. I'll close then.

@karelz karelz modified the milestones: 2.2, 3.0 Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment