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

JIT: Fix invalid zero-init suppression for untracked variables #91580

Merged
merged 2 commits into from
Sep 9, 2023

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Sep 5, 2023

optRemoveRedundantZeroInits has logic to remove unnecessary zero inits if we can determine that the local will be zeroed in the prolog. In addition, it also has logic to suppress the prolog zero init if there is a dominating initialization already. The latter logic was trying to reason about liveness for untracked locals, which does not make sense.

Fix #91576

Also makes the handling for potential implicit control flow more precise, to counteract regressions.

optRemoveRedundantZeroInits has logic to remove unnecessary zero inits
if we can determine that the local will be zeroed in the prolog. In
addition, it also has logic to suppress the prolog zero init if there is
a dominating initialization already. The latter logic was trying to
reason about liveness for untracked locals, which does not make sense.

Fix dotnet#91576
@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 5, 2023
@ghost ghost assigned jakobbotsch Sep 5, 2023
@ghost
Copy link

ghost commented Sep 5, 2023

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

Issue Details

optRemoveRedundantZeroInits has logic to remove unnecessary zero inits if we can determine that the local will be zeroed in the prolog. In addition, it also has logic to suppress the prolog zero init if there is a dominating initialization already. The latter logic was trying to reason about liveness for untracked locals, which does not make sense.

Fix #91576

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch changed the title JIT: Fix invalid zero-init supression for untracked variables JIT: Fix invalid zero-init suppression for untracked variables Sep 5, 2023
@jakobbotsch jakobbotsch added this to the 8.0.0 milestone Sep 5, 2023
@jakobbotsch
Copy link
Member Author

This sadly has some quite significant regressions, will see if I can address some of these.

@jakobbotsch jakobbotsch marked this pull request as draft September 5, 2023 13:57
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs where we now need to zero init in some cases again.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Are the handful cases where we now do less zero init ones where there were no eh successors?

@jakobbotsch
Copy link
Member Author

Are the handful cases where we now do less zero init ones where there were no eh successors?

Yep, for example in https://github.com/dotnet/runtime/blob/cf65ea21bfcb4f53a416506d43d79900413e5b1b/src/tests/JIT/Regression/JitBlue/Runtime_85892/Runtime_85892.cs vr4 no longer needs zero initing because we now realize that even though it is handler live, the handler is not reachable before the location where it is initialized.

It seems like we could be even more precise by tracking the set of locals that are actually live to handlers we have seen and only disqualifying those in this optimization, though I doubt it would be significantly better than what we have with this PR.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr superpmi-diffs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr superpmi-diffs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch
Copy link
Member Author

Opened #91839 for the Fuzzlyn failures. The jitstress failure is #91705. The superpmi-diffs failure was the "No space left on device" due to some machines in a bad state.

@jakobbotsch jakobbotsch merged commit 671d42c into dotnet:main Sep 9, 2023
197 of 204 checks passed
@jakobbotsch
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

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

@jakobbotsch jakobbotsch deleted the fix-91576 branch September 9, 2023 15:20
@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: Bad codegen with try/finally
2 participants