Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove Vector<T> fallbacks for Vector128<T> #85916

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

stephentoub
Copy link
Member

I believe the only reason we were keeping these around were some mono platforms that accelerated Vector<T> but not Vector128<T>...

@fanyang-mono, @radekdoulik, are we able to remove these now?

@ghost
Copy link

ghost commented May 8, 2023

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

Issue Details

I believe the only reason we were keeping these around were some mono platforms that accelerated Vector<T> but not Vector128<T>...

@fanyang-mono, @radekdoulik, are we able to remove these now?

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Memory

Milestone: -

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR warms my hart, I hope we can remove this code path ;)

@stephentoub
Copy link
Member Author

@SamMonoRT, any insights here?

@fanyang-mono
Copy link
Member

In .NET8, we worked on intrinsifying Vector128 for mini JIT on arm64, but we haven't do that for amd64. I would hold on doing this, until that work is finished.

@stephentoub
Copy link
Member Author

Thanks. Do we have an ETA for appx when that will happen?

@radekdoulik
Copy link
Member

On wasm this should be fine, the Vector128 coverage is pretty good. I can fill in if anything would regress.

@SamMonoRT
Copy link
Member

SamMonoRT commented May 12, 2023

In .NET8, we worked on intrinsifying Vector128 for mini JIT on arm64, but we haven't do that for amd64. I would hold on doing this, until that work is finished.

@fanyang-mono - We implemented a few Vector128 APIs on AMDx64 via our intern last year. Do we have a list of remaining ones ? -- Is this tracking issue accurate/updated - #66392 ?

@stephentoub - AMDx64 is low priority for us, but we will evaluate and consider implementing remaining ones (based on Fan's response) this summer. I wouldn't expect that to be completed prior to Preview 7 at this time.

@fanyang-mono
Copy link
Member

The intern added support for Vector128 support with LLVM as backend for AMD64 last summer. We used the doc project file to track at that time. I will create an issue to clarify this, as what I remembered that most of the API's should be supported.

@fanyang-mono
Copy link
Member

Thanks. Do we have an ETA for appx when that will happen?

I don't know atm, maybe in .NET9 time frame, but can't say for sure.

@stephentoub
Copy link
Member Author

@fanyang-mono, are you saying then that it's important these Vector<T> code paths remain here for .NET 8? 😦

@fanyang-mono
Copy link
Member

@fanyang-mono, are you saying then that it's important these Vector<T> code paths remain here for .NET 8? 😦

I just discussed this topic with my manager Sam, we understood the importance of this code change and are currently evaluating our options. Will keep you posted. The work is tracked by #86272

@stephentoub
Copy link
Member Author

To be clear, we've lived with it for this long, we can live with it for a bit longer if necessary. It'd just be really nice to get rid of :-)

@stephentoub
Copy link
Member Author

To at least make some progress here, I will ifdef these code paths to just be for mono for now rather than deleting them entirely. Then at least for coreclr and nativeaot it won't impact corelib binaries for .NET 8.

ifdef them to mono as they're not used by coreclr or nativeaot.  They can be deleted instead once mono's Vector128 support improves.
@stephentoub stephentoub force-pushed the removevectorfallback branch from 3de41d9 to f4817f9 Compare June 8, 2023 01:56
@stephentoub
Copy link
Member Author

I've updated the PR to just ifdef the Vector<T> paths to be mono-specific instead of outright deleting them.
cc: @tannergooding

@stephentoub
Copy link
Member Author

@SamMonoRT, this should be a nop for mono so I assume you're ok with it. We can delete the code paths and improve maintainability when mono is ready

@adamsitnik, @EgorBo, or @tannergooding, mind reviewing?

@SamMonoRT
Copy link
Member

@fanyang-mono - need to make sure we track the cleanup of this file once the implementations are completed on our side.

@fanyang-mono
Copy link
Member

@fanyang-mono - need to make sure we track the cleanup of this file once the implementations are completed on our side.

Updated #86272

@stephentoub stephentoub merged commit eef8dd9 into dotnet:main Jun 8, 2023
@stephentoub stephentoub deleted the removevectorfallback branch June 8, 2023 13:48
@stephentoub
Copy link
Member Author

Thanks, all

@ghost ghost locked as resolved and limited conversation to collaborators Jul 8, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants