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

Update Preview/Experimental Attributes in Intrinsics #105579

Merged
merged 10 commits into from
Jul 30, 2024

Conversation

jeffhandley
Copy link
Member

@jeffhandley jeffhandley commented Jul 26, 2024

This PR includes 4 changes to preview/experimental attributes (each as a separate commit):

  1. Arm.Sve is switched from [RequiresPreviewFeatures] to [Experimental] with SYSLIB5003
    • These are experimental APIs that do not require preview features from other parts of the product
    • This allows usage of these APIs by only suppressing SYSLIB5003 instead of fully opting into preview features across the stack
  2. X86.AvxVnni is no longer in preview; the APIs are now stable
  3. X86.X86Base.DivRem is switched from [RequiresPreviewFeatures] to [Experimental] with SYSLIB5004
  4. GenericVectorTests had an unnecessary [RequiresPreviewFeatures] left from when generic math was preview

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jul 26, 2024

This comment was marked as resolved.

1 similar comment

This comment was marked as duplicate.

@jeffhandley jeffhandley added this to the 9.0.0 milestone Jul 26, 2024
@jeffhandley jeffhandley added area-System.Runtime.Intrinsics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jul 26, 2024
Copy link
Contributor

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

@stephentoub
Copy link
Member

X86.X86Base.DivRem does not require preview features. There are still JIT opportunities to improve performance, but users should not need to opt into preview features to use the API

The presence of this attribute seems to be how we're communicating it's not actually ready for folks to use, e.g.
#99747 (comment)
Has this changed?

@jeffhandley
Copy link
Member Author

X86.X86Base.DivRem does not require preview features

@kunalspathak and @tannergooding indicated in an offline chat last week that we could remove the preview/experimental annotation, but nothing has changed on its performance. Thanks for pointing to that discussion, @stephentoub.

@tannergooding, based on that thread, I suggest we retain an annotation on this API, but switch it from [RequiresPreviewFeatures] to [Experimental].

@tannergooding
Copy link
Member

The consideration here is that DivRem is stable; however, there's some additional JIT work needed to ensure it performs "optimally" and that may be unexpected to users given that it is a platform specific hardware intrinsic.

Its something that could probably be just called out in the documentation as its not overall different from other cases where alternative patterns or APIs may be better performing. -- For example, using Sse41.BlendVariable can often be worse than using Vector128.ConditionalSelect as its less portable and more strict on what needs to be emitted.

@tannergooding
Copy link
Member

That is, I think that overall its fine at this point to remove the attribute

We have many cases where a given platform specific intrinsic may be subtly "worse" than using an xplat API (in this case its X86Base.DivRem vs int.DivRem). So while there's more JIT work to be done, the API itself is still considered stable and so continuing to mark it as Experimental or Preview may give a worse impression than simply covering the consideration in docs or with an analyzer (which we have approved, just not yet implemented)

That's not a strong preference though, so if anyone want to push back I'm fine with keeping it. I would prefer switching to Experimental though if we do keep any attribute, as that's less strict and easier to document that its just because perf may be suboptimal compared to T.DivRem in some cases

@stephentoub
Copy link
Member

So while there's more JIT work to be done

Is it a lot more work?

I don't have a strong opinion about it either, it just seems we marked it as preview/experimental because that work hadn't been done, but now we're removing the attribution without having done the work. In which case I question what changed, e.g. did we change our minds, did enough work happen to make us more comfortable with it, will we always dissuade folks from using the API (in which case maybe it shouldn't have been added publicly in the first place and should possibly be removed), etc.

@tannergooding
Copy link
Member

More general work has happened and its better than it had been, but its still not "quite there". The work could likely be finished in early .NET 10 if it was scheduled as top down work, rather than being left as "nice to have".

I'd be happy with changing it to be Experimental for .NET 9 as well; just noting that I don't think its strictly necessary given the above and the general considerations that platform specific APIs are not always the most optimal way to do a given operation.

@jeffhandley
Copy link
Member Author

Thanks @stephentoub and @tannergooding. I'll switch DivRem over to [Experimental] for now (9.0.0), and before we remove it, we'll try to reach a point of understanding more broadly what we want to do with intrinsics APIs in this perf situation.

@jeffhandley jeffhandley merged commit 50bc6a1 into dotnet:main Jul 30, 2024
149 checks passed
@jeffhandley jeffhandley deleted the jeffhandley/intrinsics-preview branch July 30, 2024 23:20
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants