Skip to content

[release/8.0-staging] Ensure that Vector512 uses the same patterns as Vector64/128/256 #98115

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

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 7, 2024

Backport of #97990 to release/8.0-staging

/cc @tannergooding

Customer Impact

  • Customer reported
  • Found internally

Customers attempting to use the Vector512<T> Create(T[] values, int index) API will see incorrect results for non-zero indices.

Regression

  • Yes
  • No

This is a net new type in .NET 8

Testing

Additional test coverage was added to ensure the scenario was correctly covered. Vector512 was a net new type following the existing API surface area of the Vector64/128/256 APIs. The surface area is quite expansive and the tests for this particular scenario were missing.

Risk

Low. The Vector512 APIs were checked in with a slightly different implementation than had previously been used for Vector64, Vector128, and Vector256, the fix was to ensure we followed the same well-tested and consistent behavior. The failure was itself caused by a simple ordering issue (effectively the code was doing Add(AsByte(x), y)) when it should have been doing AsByte(Add(x, y))

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 7, 2024
@tannergooding tannergooding 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 labels Feb 7, 2024
@ghost
Copy link

ghost commented Feb 7, 2024

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 #97990 to release/8.0-staging

/cc @tannergooding

Customer Impact

  • Customer reported
  • Found internally

Customers attempting to use the Vector512<T> Create(T[] values, int index) API will see incorrect results for non-zero indices.

Regression

  • Yes
  • No

This is a net new type in .NET 8

Testing

Additional test coverage was added to ensure the scenario was correctly covered. Vector512 was a net new type following the existing API surface area of the Vector64/128/256 APIs. The surface area is quite expansive and the tests for this particular scenario were missing.

Risk

Low. The Vector512 APIs were checked in with a slightly different implementation than had previously been used for Vector64, Vector128, and Vector256, the fix was to ensure we followed the same well-tested and consistent behavior. The failure was itself caused by a simple ordering issue (effectively the code was doing Add(AsByte(x), y)) when it should have been doing AsByte(Add(x, y))

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Runtime.Intrinsics

Milestone: -

@jeffhandley
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Contributor

@tannergooding @jeffhandley today is code complete for the March release. If this PR meets all the servicing criteria, please merge it before 4pm.

@tannergooding
Copy link
Member

Extra platforms failures are CI timeouts due to device disconnections. They are fairly common for that leg.

@jeffhandley, I think this is ready to merge

@tannergooding tannergooding merged commit 00c95a6 into release/8.0-staging Feb 12, 2024
@tannergooding tannergooding deleted the backport/pr-97990-to-release/8.0-staging branch February 12, 2024 21:03
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2024
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.

4 participants