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

Fix Avx2 helpers #85275

Merged
merged 12 commits into from
Apr 25, 2023
Merged

Fix Avx2 helpers #85275

merged 12 commits into from
Apr 25, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Apr 24, 2023

Unblocks dotnet-optimization flow and #84711. Includes a dotnet-optimization update to confirm this it avoids the issue discussed in #84635. This fixes that issue by preventing R2R for any Avx2 helpers which assume they are called with Avx2 enabled.

@davidwrighton taking a shot at fixing this by skipping R2R for the instances that I was able to find by inspection [edit: using the platform analyzer with some tweaks]. Just a draft for now since I'm not sure if this is the approach we want to take - I just wanted to see what happens in ci.

dotnet-maestro bot and others added 8 commits April 21, 2023 17:36
…otnet-optimization build 20230420.15

optimization.linux-arm64.MIBC.Runtime , optimization.linux-x64.MIBC.Runtime , optimization.windows_nt-arm64.MIBC.Runtime , optimization.windows_nt-x64.MIBC.Runtime , optimization.windows_nt-x86.MIBC.Runtime , optimization.PGO.CoreCLR
 From Version 1.0.0-prerelease.23175.4 -> To Version 1.0.0-prerelease.23220.15
This reverts commit ab43da4.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 24, 2023
@ghost ghost assigned sbomer Apr 24, 2023
@MihaZupan
Copy link
Member

Just to confirm I understand the rules for S.P.CoreLib, a method like Vector128.ShuffleUnsafe may end up calling the Shuffle fallback when running on ARM/WASM, but it will never call the fallback on a platform with Ssse3 support, because that is treated differently?

I'm asking about ShuffleUnsafe specifically because the callers do expect different behavior depending on the platform.

@sbomer
Copy link
Member Author

sbomer commented Apr 25, 2023

That matches my read of the rules, since they mention R2R code checks for Ssse3 support at runtime (whereas Avx2 is assumed to be unavailable), but @davidwrighton would know best.

@davidwrighton
Copy link
Member

@sbomer I believe this looks good for now. I'm working on a more permanent fix, that will likely result in changes to all of these places to a slightly different form, but getting a fix in before the snap would be better.

@MihaZupan The "rules" indicate that the ShuffleUnsafe method has a problem, but as @sbomer notes, there are also some implementation choices in the current default behavior of Crossgen2 which make the ShuffleUnsafe method reliably functional.

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Looks good for now. I'm working on an analyzer we can turn on all the time, and some crossgen2 tweaks to make it easier to write these things. As it is the BypassReadyToRun changes will pessimize some of the work done for the ASP.NET composite container scenario, but I prioritize working over not working. @dotnet/crossgen-contrib

@sbomer
Copy link
Member Author

sbomer commented Apr 25, 2023

Wasm failures look unrelated. I filed #85354 for these.

@MihaZupan
Copy link
Member

Is something like this also problematic?

Vector128<byte> source =
Sse2.IsSupported ? Sse2.PackUnsignedSaturate(source0, source1) :
AdvSimd.IsSupported ? AdvSimd.ExtractNarrowingSaturateUpper(AdvSimd.ExtractNarrowingSaturateLower(source0.AsUInt16()), source1.AsUInt16()) :
PackedSimd.ConvertNarrowingUnsignedSaturate(source0, source1);

Could we end up calling into PackedSimd on ARM for example?

@davidwrighton
Copy link
Member

@MihaZupan, technically ... yes, except you'd have to pass a very surprising set of switches to crossgen2, which nobody is likely to do. I've a followon fix (#85481) which should make this very hard to mess up, and will protect against that kind of case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants