Revert "JIT: Remove side effect detection quirk in loop hoisting (#118463)" with some improvements #119138
Conversation
…net#118463)" This reverts commit e0b14e9. That change is not correct as it removes the safe guard that prevents hoisting out of conditionally executed blocks.
There was a problem hiding this comment.
Pull Request Overview
This PR reverts a previous change that incorrectly removed a safeguard preventing loop hoisting from conditionally executed blocks. The reverted change (commit e0b14e9) was causing incorrect JIT optimization behavior where expressions with side effects could be hoisted out of conditionally executed code blocks, leading to potential runtime errors.
- Restores the
m_beforeSideEffect = falsesafeguard in loop hoisting logic - Adds comprehensive comments explaining why the safeguard is necessary
- Includes a regression test to prevent similar issues in the future
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/optimizer.cpp | Reverts the loop hoisting logic and adds detailed comments explaining the safeguard mechanism |
| src/tests/JIT/Regression/JitBlue/Runtime_119061/Runtime_119061.cs | Adds regression test with conditional block and side-effect operations to verify fix |
| src/tests/JIT/Regression/JitBlue/Runtime_119061/Runtime_119061.csproj | Test project configuration for the regression test |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Note that this isn't a straight up reversion since I noticed another potential issue while looking at this code. |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17459176149 |
This reverts commit e0b14e9. That change is not correct as it removes the safe guard that prevents hoisting throwing expressions out of conditionally executed blocks.
Also renames
m_beforeSideEffecttom_canHoistSideEffectsto capture more precisely what it represents and why the conditionality of the blocks play a role. Also adds a test.Fix #119061