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

[release/7.0] Ensure that the shuffle zero mask copies all bits #86453

Merged
merged 4 commits into from
May 22, 2023

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented May 18, 2023

Backport of a fix from #85129 to release/7.0

/cc @dotnet/area-system-runtime-intrinsics

Customer Impact

Customers expecting elements in the upper 128-bits of a Vector256<T> to be zero'd by the shuffle will not see the correct behavior and may end up with non-zero result for the corresponding element.

Testing

A unit test covering the faulty behavior was added to validate the fix.

Risk

Low. The bug was for a new API introduced in .NET 7 and is covering a specific edge case scenario where the mask was only copying the lower 128-bits. We've had the fix in .NET for several previews now with no issues.

This resolves #85132

@tannergooding tannergooding added Servicing-consider Issue for next servicing release review area-System.Runtime.Intrinsics labels May 18, 2023
@ghost ghost assigned tannergooding May 18, 2023
@ghost
Copy link

ghost commented May 18, 2023

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

Issue Details

Backport of a fix from #85129 to release/7.0

/cc @dotnet/area-system-runtime-intrinsics

Customer Impact

Customers expecting elements in the upper 128-bits of a Vector256<T> to be zero'd by the shuffle will not see the correct behavior and may end up with non-zero result for the corresponding element.

Testing

A unit test covering the faulty behavior was added to validate the fix.

Risk

Low. The bug was for a new API introduced in .NET 7 and is covering a specific edge case scenario where the mask was only copying the lower 128-bits. We've had the fix in .NET for several previews now with no issues.

Author: tannergooding
Assignees: -
Labels:

Servicing-consider, area-System.Runtime.Intrinsics

Milestone: -

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Approved assuming the build errors get addressed and tactics approves the servicing request.

@tannergooding
Copy link
Member Author

assuming the build errors get addressed

Should be fixed. I forgot to exclude the V512 specific coverage from the test (V512 is .NET 8 only)

@tannergooding
Copy link
Member Author

CI failures are all known, they are:

The latter is the most prevalent by far and surfaces as either:

Unhandled exception. System.Exception: ClrProductVersion must match. Expected: "7.0.7", received: "7.0.8"

-or-

The command output did not contain expected result: '7.0.8'
File Name: /Users/runner/work/1/s/artifacts/tests/Release/ha/5ulxvfrd.jhy/StandaloneApp/bin/StandaloneApp
Arguments:
Exit Code: 0
StdOut:
Hello World!


.NET 7.0.7


StdErr:

@tannergooding tannergooding added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 20, 2023
@tannergooding tannergooding merged commit 2a63c9d into dotnet:release/7.0-staging May 22, 2023
@ayende
Copy link
Contributor

ayende commented May 27, 2023

What release will this go into?

@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants