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/9.0] JIT: Null out SSA def nodes upon removal in RBO #108548

Open
wants to merge 4 commits into
base: release/9.0
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 4, 2024

Backport of #108530 to release/9.0

/cc @amanasifkhalid

Customer Impact

  • Customer reported
  • Found internally

Various JIT optimizations can remove stores to local variables. If the JIT removes a store to a variable that's tracked by SSA, the removed store node might be tracked by an SSA definition, which needs to be invalidated; otherwise, we run the risk of referencing invalid IR later on. When optimizing redundant branches, the JIT may remove a statement containing a store node after SSA data structures have been initialized, so we must take care to maintain them here.

Regression

  • Yes
  • No

As far as I can tell, the lack of SSA definition maintenance has been around for several releases, though recent expansion of SSA-based optimizations may have revealed this.

Testing

The issue of referencing invalid SSA definition nodes was exposed by a test case generated by one of our fuzzing tools. Upon further inspection, our existing test suites have been referencing invalid IR nodes during SSA-based opts, but the overwhelming majority of the time, the JIT's heuristics bail for these nodes, hence why we didn't hit this failure earlier. I've added some debug logic during store node removal in redundant branch opts to overwrite the removed node with garbage values, so future attempts to read removed nodes will trigger asserts.

Risk

Low, based on the fact that it took us this long to find a code shape that exposes this failure. The fix is simple: We null the pointer to the removed store node in its corresponding SSA definition node. Existing call sites that use SSA definitions are already expected to check for null IR nodes to handle cases where the store is removed, so this fix is unlikely to regress JIT behavior elsewhere.

@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 Oct 4, 2024
Copy link
Contributor

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

@amanasifkhalid
Copy link
Member

@AndyAyersMS PTAL, thanks!

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.

lgtm. please get a code review. we will take for consideration in 9 GA

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Oct 4, 2024
@jeffschwMSFT jeffschwMSFT added this to the 9.0.0 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants