JIT: Fix for IV opt to preserve updates to a counter that is live into an EH handler#129058
JIT: Fix for IV opt to preserve updates to a counter that is live into an EH handler#129058JulieLeeMSFT wants to merge 2 commits into
Conversation
…handlers optLocalHasNonLoopUses relied on lvDoNotEnregister to detect locals live into exceptional exits. PR dotnet#127932 decoupled DNER from liveness (DNER is now set during LSRA/lowering, after optInductionVariables runs), so an induction variable live into a catch handler was no longer detected and its in-loop self-update was removed by optRemoveUnusedIVs, producing wrong results under full-opt codegen (crossgen2, jitstress, AggressiveOptimization). Add an explicit lvTracked && IsLiveInOutOfHandler() check, since VisitRegularExitBlocks deliberately excludes handler blocks. Update two stale comments referencing the old DNER behavior. Fixes dotnet#128392. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness hole in CoreCLR JIT induction-variable optimizations where a loop IV update could be removed even though the IV is live into an EH handler (e.g., used by a catch). The change makes optLocalHasNonLoopUses explicitly treat “live into handler” as a non-loop use, preventing optRemoveUnusedIVs from dropping the in-loop update.
Changes:
- Update
optLocalHasNonLoopUsesto conservatively returntruewhen a tracked local isIsLiveInOutOfHandler(). - Adjust a comment in IV widening logic to reflect that exceptional-exit-live IVs are not profitable candidates (rather than relying on DNER being set).
- Add a JIT regression test (Runtime_128392) and include it in the regression csproj.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/inductionvariableopts.cpp | Adds an explicit EH-handler liveness check in optLocalHasNonLoopUses to avoid removing IV updates that are observable via exceptional control flow. |
| src/tests/JIT/Regression/JitBlue/Runtime_128392/Runtime_128392.cs | New xUnit regression covering an IV read in a catch after an exception thrown mid-loop. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new regression test source file in the build. |
|
Is this 11.0 or 10.0? |
This is a regression from #127932 |
| if (varDsc->lvDoNotEnregister) | ||
| { | ||
| // This filters out locals that may be live into exceptional exits. | ||
| // We cannot reason precisely about uses of locals that are not | ||
| // enregisterable, so be conservative. | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I think this check should just be removed.
There was a problem hiding this comment.
@copilot, please remove this check if (varDsc->lvDoNotEnregister).
|
Can you introduce a similar assert as runtime/src/coreclr/jit/inductionvariableopts.cpp Lines 471 to 489 in 01cd5cc at the end of |
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
@copilot, please add this code to the end of optLocalHasNonLoopUses before |
optLocalHasNonLoopUsesrelied onlvDoNotEnregisterto detect locals live into exceptional exits. PR #127932 decoupled DNER from liveness (DNER is now set during LSRA/lowering, afteroptInductionVariablesruns), so an induction variable live into a catch handler was no longer detected and its in-loop self-update was removed byoptRemoveUnusedIVs, producing wrong results under full-opt codegen (crossgen2, jitstress, AggressiveOptimization).Add an explicit
lvTracked && IsLiveInOutOfHandler()check, sinceVisitRegularExitBlocksdeliberately excludes handler blocks.Fixes #128392.