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/8.0-rc2] Don't generate AddMask as it requires more explicit consideration of semantics #92308

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 19, 2023

Backport of #92282 to release/8.0-rc2

/cc @tannergooding

Customer Impact

The customer will experience incorrect codegen that results in the code doing something different than they intended when summing Vector512<T>s created via certain patterns with operator+. Customer reported in #92261.

Testing

Manual validation that the reported customer scenario produces the expected codegen.

Risk

Low. This is simply removing an optimization that shouldn't have been included in the first place as it was incomplete. Additional work was required to ensure that the kadd code generation worked as expected for adding two masks together.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 19, 2023
@ghost
Copy link

ghost commented Sep 19, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #92282 to release/8.0

/cc @tannergooding

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

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

area-CodeGen-coreclr

Milestone: -

@carlossanlop
Copy link
Member

@tannergooding is it your intention to target RC2? There's still time. The hard due date for merging is on Sept 24th. If yes, then you need to retarget your PR to release/8.0-rc2 and you need Tactics approval for merging. Once merged, the bot will automatically flow your change to the base release/8.0 branch for you.

cc @jeffhandley @artl93

@tannergooding
Copy link
Member

This is a JIT side issue, so it likely needs determination from @JulieLeeMSFT or @jakobbotsch

Given it's a correctness issue, it might be better to target RC2

@jakobbotsch
Copy link
Member

Agreed, let's target rc2 for this one since it's customer reported.

@jakobbotsch jakobbotsch changed the base branch from release/8.0 to release/8.0-rc2 September 19, 2023 22:13
@jakobbotsch jakobbotsch added the Servicing-consider Issue for next servicing release review label Sep 19, 2023
@jakobbotsch jakobbotsch changed the title [release/8.0] Don't generate AddMask as it requires more explicit consideration of semantics [release/8.0-rc2] Don't generate AddMask as it requires more explicit consideration of semantics Sep 19, 2023
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

please get a code review

@carlossanlop
Copy link
Member

carlossanlop commented Sep 20, 2023

Approved by Tactics via email.

The RC2 branch had a generalized failure for which we just merged a fix, so I updated this PR to ensure the CI is clean.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 20, 2023
@carlossanlop carlossanlop added this to the 8.0.0 milestone Sep 20, 2023
@carlossanlop carlossanlop merged commit 09429dc into release/8.0-rc2 Sep 20, 2023
108 of 112 checks passed
@carlossanlop carlossanlop deleted the backport/pr-92282-to-release/8.0 branch September 20, 2023 06:12
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants