Skip to content

JIT: add missing xarch RMW case #92252

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 19, 2023
Merged

JIT: add missing xarch RMW case #92252

merged 1 commit into from
Sep 19, 2023

Conversation

@AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Sep 18, 2023

Handle the case where we're indirectly updating a local with a value that is not a constant.

Fixes #92218.

@AndyAyersMS
JIT: add missing xarch RMW case
edad3b9 
Handle the case where we're indirectly updating a local with a value
that is not a constant.

Fixes dotnet#92218.
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 18, 2023
@ghost ghost assigned AndyAyersMS Sep 18, 2023
@ghost
Copy link

ghost commented Sep 18, 2023

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

Issue Details

Handle the case where we're indirectly updating a local with a value that is not a constant.

Fixes #92218.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 18, 2023

@BruceForstall PTAL
cc @dotnet/jit-contrib

This is a regression from .NET 7, so we'll need to make the case for a backport.

BruceForstall
BruceForstall approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

Probably should run jitstress

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 19, 2023

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 19, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Sep 19, 2023
Closed
@jakobbotsch
Copy link
Member

jakobbotsch commented Sep 19, 2023

The issue looks like the same as #41073, where it seems we elected not to fix it in emitHandleMemOp. Does the reasoning given in its PR make sense for this situation too?

@AndyAyersMS AndyAyersMS mentioned this pull request Sep 19, 2023
Closed
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 19, 2023

The issue looks like the same as #41073, where it seems we elected not to fix it in emitHandleMemOp. Does the reasoning given in its PR make sense for this situation too?

My fix is in emitInsRMW so is consistent with #50669 (the fix for #41073), which added the S_I case.

jitstress failures also happening in main (#92202 ).

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 19, 2023

Build analysis doesn't seem to be tracking known jitstress issues, so will have to "merge on red".

@AndyAyersMS AndyAyersMS merged commit 67dbbeb into dotnet:main Sep 19, 2023
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 19, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6239547972

@github-actions github-actions bot mentioned this pull request Sep 19, 2023
Merged
@jakobbotsch
Copy link
Member

jakobbotsch commented Sep 19, 2023

Build analysis doesn't seem to be tracking known jitstress issues, so will have to "merge on red".

The issues with the JSON blobs need to have the "Known Build Error" label to be included in the analysis. I've added it to #92202

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 19, 2023

Build analysis doesn't seem to be tracking known jitstress issues, so will have to "merge on red".

The issues with the JSON blobs need to have the "Known Build Error" label to be included in the analysis. I've added it to #92202

Aha. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AndyAyersMS AndyAyersMS

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect NRE during operations on structures in method marked with AgressiveOptimization

3 participants

@AndyAyersMS @jakobbotsch @BruceForstall