-
Notifications
You must be signed in to change notification settings - Fork 5.2k
JIT: handle commas during inline return value substitution #119160
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a JIT assertion failure during inline return value substitution when handling comma operators. The issue occurred when a conditional jump (jtrue) had a comma operation as its condition, causing the JIT to fail during flow graph optimization.
Key changes:
- Adds handling for comma operations in conditional statements during inline substitution
- Extracts side effects from comma operations into separate statements
- Includes a regression test to prevent this issue from reoccurring
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/coreclr/jit/fginline.cpp |
Adds logic to handle comma operations in jtrue statements by extracting side effects and splicing out the comma |
src/tests/JIT/Regression/JitBlue/Runtime_119508/Runtime_119508.cs |
Regression test that reproduces the original JIT assertion failure scenario |
src/tests/JIT/Regression/JitBlue/Runtime_119508/Runtime_119508.csproj |
Project file for the regression test with optimization enabled |
@dotnet/jit-contrib PTAL |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/tests/JIT/Regression/JitBlue/Runtime_119508/Runtime_119508.cs
Outdated
Show resolved
Hide resolved
Also we don't really need to extra all side effects, just tunnel down through until we find a compare or constant. Maybe I should rework this as a split. |
@EgorBo updated; can you take one more look? |
@EgorBo ping |
/backport to release/10.0 |
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17407078615 |
Fixes #119058.