JIT: bail on flipping post-layout JTRUE when reversal needs new IR#127746
JIT: bail on flipping post-layout JTRUE when reversal needs new IR#127746Copilot wants to merge 6 commits into
Conversation
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/5d85b59c-0590-49b1-9854-94014903fa1a Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c3a02af3-6597-451c-ad3c-267603ddb8ba Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adjusts late (post-LSRA) block-flipping in the CoreCLR JIT to avoid creating new IR nodes after LIR has been formed, by introducing an in-place-only condition reversal helper. This prevents LIR integrity failures when condition reversal would otherwise require synthesizing new IR (e.g., wrapping a non-compare in == 0) and also handles GT_COPY/GT_RELOAD wrappers introduced by LSRA on GT_JTRUE operands.
Changes:
- Add
Compiler::gtTryReverseCond(GenTree*) -> boolto reverse supported conditions in-place (relops,JCC/SETCC,JCMP/JTEST, integral constants) without allocating new IR. - Make
gtReverseConda thin wrapper that preserves existing behavior by falling back to creatingGT_EQ(tree, 0)when in-place reversal isn’t possible. - Update
optOptimizePostLayoutto (1) peelGT_COPY/GT_RELOADoffGT_JTRUEoperands and (2) skip flipping a block when in-place reversal isn’t possible, rather than mutating LIR.
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 | Switch post-layout flipping to use in-place-only reversal and bail out when reversal would require new IR; skip LSRA GT_COPY/GT_RELOAD wrappers for GT_JTRUE. |
| src/coreclr/jit/gentree.cpp | Introduce gtTryReverseCond and refactor gtReverseCond to use it, keeping the existing fallback semantics for other callers. |
| src/coreclr/jit/compiler.h | Declare the new gtTryReverseCond helper on Compiler. |
|
cc @dotnet/jit-contrib PTAL @dhartglassMSFT |
|
@copilot add the bug repro from 127745 as a JIT regression test |
… assert) Co-authored-by: dhartglassMSFT <248563697+dhartglassMSFT@users.noreply.github.com>
Done in the latest commit. Added |
There was a problem hiding this comment.
I think it is unlikely that this test reproduces the original problem. Fuzzlyn found that we need a collectible ALC for the test (that impacts our codegen for static access), and that's a bit of a pain to set up correctly in the regression tests. I would probably just skip the regression test and rely on the Fuzzlyn testing.
There was a problem hiding this comment.
sounds good, change LGTM, if you need another signoff after presumably removing the cs and csproj changes let me know
optOptimizePostLayoutruns after LSRA and calledgtReverseCond, which can fall back to wrapping the operand in a freshGT_EQ(tree, 0). The newGT_INT_CONchild is never inserted into the LIR, trippingfound use of a node that is not in the LIR sequencein checked builds (reported by Fuzzlyn on linux-arm32). The fallback was also reached whenever LSRA inserted aGT_COPY/GT_RELOADon top of the JTRUE operand, since the wrapper is not itself a comparison.Description
gentree.cpp/compiler.h— ExtractgtTryReverseCond(GenTree*) -> boolthat reverses compares,JCC/SETCC,JCMP/JTEST, and integral constants in place.gtReverseCondbecomes a thin wrapper that adds theGT_EQ(tree, 0)fallback when in-place reversal isn't possible, preserving semantics for all existing callers.optimizer.cpp— InoptOptimizePostLayout, peelGT_COPY/GT_RELOADoff theGT_JTRUEoperand viagtSkipReloadOrCopy(), then usegtTryReverseCondand skip flipping the block if it returnsfalserather than mutating the LIR.src/tests/JIT/Regression/JitBlue/Runtime_127745— Added a JIT regression test using the reduced repro from the Fuzzlyn report: a method with a do-while loop, try-finally, and a bitwise-OR of two comparisons that produces aSETCCresult under aJTRUE.