JIT: Preserve exception order in loop hoisting - #133642
Open
AndyAyersMS wants to merge 1 commit into
Open
Conversation
Loop hoisting could move a throwing expression ahead of an invariant but non-hoistable throwing expression, changing the observed exception. Block further side-effecting hoists whenever a throwing expression cannot itself be hoisted. Fixes dotnet#133585 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69df99f0-1e6a-49a2-8061-9611e90c24bc
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified in the reviewed changes.
Pull request overview
Fixes JIT loop-hoisting exception ordering by preventing later throwing expressions from being hoisted prematurely.
Changes:
- Closes the hoisting barrier for non-hoistable throwing expressions.
- Adds regression coverage for bounds-check and division exceptions.
File summaries
| File | Description |
|---|---|
src/tests/JIT/Regression/JitBlue/GitHub_7147/GitHub_7147.cs |
Adds regression coverage verifying exception order. |
src/coreclr/jit/optimizer.cpp |
Preserves exception ordering during loop hoisting. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
Member
Author
|
In this case we have an invariant bounds checked array access. Seems like we ought to be able to hoist such things, but we can't, and so we then need to block any other possible exception hoists. @jakobbotsch PTAL |
Member
Author
|
This fails the same way in 9, 10, and 11, so is not a recent regression. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #133585.
Loop hoisting previously kept its exception-ordering barrier open for an
invariant but non-hoistable throwing expression. This allowed a later throwing
expression to be hoisted ahead of it. Close the barrier whenever a throwing
expression cannot itself be hoisted.
Add coverage to the existing loop-hoisting exception-ordering regression test.
Validation:
build.cmd clr.jit -c checkedRegression_o_1: new case fails with the baseline JIT and passes with the changed JITNote
This pull request description was generated with GitHub Copilot.