Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Account for global stores in hoisting properly #67213

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Account for global stores in hoisting properly #67213

merged 3 commits into from
Mar 31, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Mar 28, 2022 •
edited
Loading

We were not doing so in all cases, which could lead to reordering a throw with a store.

Small diffs where we no longer hoist across a global store.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 28, 2022
@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 Mar 28, 2022
@ghost
Copy link

ghost commented Mar 28, 2022

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

Issue Details

We were not doing so in all cases, which could lead to reordering a throw with a store.

We're expecting some small diffs where we no longer hoist across a global store.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution
Milestone: -

@SingleAccretion
Copy link
Contributor Author

Assuming CI failures are not related:

  1. Linux ARM - System.Text.Json.SourceGeneration.Roslyn3.11.Tests crashes due to an assertion on linux/arm64/debug #65817.
  2. OSX x64 - Profiler.Tests.GCBasicTests test failure #67106.
  3. Windows ARM64 - same failure mode as here (which was for a comment-only change).

@SingleAccretion SingleAccretion marked this pull request as ready for review March 28, 2022 08:04
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@SingleAccretion
Take HWI stores into account in hoisting
c8f8869
@SingleAccretion
Also fix ordinary stores while we're at it
99c00c1 
The contract for whether the LHS of a global ASG(IND, ...)
should be marked with GLOB_REF is not clearly specified.
@SingleAccretion
Add tests
794a7bb
@SingleAccretion SingleAccretion force-pushed the Store-Effects-Fixes-Hoisting branch from 0d0626b to 794a7bb Compare March 29, 2022 18:13
@BruceForstall BruceForstall merged commit 7f94e6e into dotnet:main Mar 31, 2022
@SingleAccretion SingleAccretion deleted the Store-Effects-Fixes-Hoisting branch March 31, 2022 20:45
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Apr 29, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@EgorBo EgorBo EgorBo approved these changes

@BruceForstall BruceForstall BruceForstall approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SingleAccretion @EgorBo @BruceForstall