Fix side-effect reordering when folding constant-zero-mask BlendVariable#130946
Open
tannergooding wants to merge 5 commits into
Open
Fix side-effect reordering when folding constant-zero-mask BlendVariable#130946tannergooding wants to merge 5 commits into
tannergooding wants to merge 5 commits into
Conversation
When gtFoldExprHWIntrinsic folds a BlendVariable with a constant all-zero mask down to op1, gtWrapWithSideEffects(op1, op2) hoisted op2's side effects ahead of op1 even though op2 is evaluated after op1. Only fold when op2 has no side effects and return op1 directly; otherwise bail and let VN handle it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 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.
Pull request overview
This PR updates RyuJIT’s gtFoldExprHWIntrinsic constant-folding for BlendVariable/BlendVariableMask when the mask is a constant all-zero, to avoid unsafe operand dropping / side-effect reordering, and adds a regression test under JitBlue.
Changes:
- Adjust
BlendVariable*folding somaskIsZeroonly returnsop1when it’s safe to dropop2. - Add a deterministic JitBlue regression test exercising the evaluation-order / exception ordering scenario.
- Add a test project file that forces
DOTNET_TieredCompilation=0for determinism.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/gentree.cpp | Updates BlendVariable* folding behavior for constant masks. |
| src/tests/JIT/Regression/JitBlue/Runtime_125431/Runtime_125431.cs | Adds regression coverage for operand evaluation order and exception behavior. |
| src/tests/JIT/Regression/JitBlue/Runtime_125431/Runtime_125431.csproj | Configures isolated execution + env var for deterministic tiering behavior. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
Bail on any op2 effect (including ordering/global refs such as volatile loads), not just GTF_SIDE_EFFECT. Switch the regression test to ConditionalFact so unsupported hardware is reported as skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Member
Author
|
CC. @dotnet/jit-contrib, @dhartglassMSFT a small side-effect preservation fix |
This was referenced Jul 17, 2026
Open
Member
Author
|
Found a couple more side effect checks that were technically incorrect, so fixed them. Given they require a non-faulting volatile load, getting a regression test added is difficult. |
When a constant-fold identity makes an operand value-dead, it may only be dropped if it has no user-visible side effect and no ordered effect. The guards checked GTF_SIDE_EFFECT (or GTF_ALL_EFFECT), so a non-faulting volatile load could be silently dropped. Use the minimal correct mask GTF_SIDE_EFFECT | GTF_ORDER_SIDEEFF for the drop-in-place folds in gtFoldExprHWIntrinsic (BlendVariable, ConditionalSelect), the AVX512 Fixup seed, and fgOptimizeMultiply mul-by-zero. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Force optimized codegen with AggressiveOptimization instead of process-isolated tiered-compilation disabling, so the test can drop its standalone project and join Regression_ro_2. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Open
3 tasks
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 #125431
gtFoldExprHWIntrinsicfolds aBlendVariable/BlendVariableMaskwith a constant mask down to one of its operands. Operands evaluate in orderop1(selectFalse),op2(selectTrue),op3(mask). When the mask is a constant all-zero, the result isop1andop2is dropped. The old code did:gtWrapWithSideEffects(op1, op2)buildsCOMMA(sideEffects(op2), op1), which hoistsop2's side effects ahead ofop1. Butop2is evaluated afterop1in source order, so this reorders them. In the repro,op1isCreateScalar(M43(...))(a call) andop2is a null-array load (throwsNullReferenceException); the fold moved the null-load exception ahead of theM43call, dropping the observable side effect ofM43(matchingDebug prints 1 line, Release prints 0).Fix: mirror the conservative
maskIsAllBitsSetbranch directly above -- only fold whenop2has no side effects and returnop1directly; otherwisebreakand let value numbering handle the value fold (which preserves side-effect ordering), the same way the neighboringmaskIsAllBitsSetandConditionalSelecthandlers already do.Added a deterministic regression test under
JitBlue/Runtime_125431(runs withDOTNET_TieredCompilation=0). Confirmed it fails on the unfixed JIT and passes with the fix; the original Fuzzlyn repro now prints the expected line before theNullReferenceException.jit-formatreports no changes.Known related gap (deferred, not fixed here): the same dropped-operand hazard exists in the shared side-effect extraction machinery.
gtNodeHasSideEffects/gtExtractSideEffListonly considerGTF_ASG | GTF_CALL | GTF_EXCEPT | GTF_MAKE_CSEand ignoreGTF_ORDER_SIDEEFF/GTF_GLOB_REF, even when passedGTF_ALL_EFFECT. A non-faulting volatile load (GLOB_REF | ORDER_SIDEEFF, noEXCEPT) can therefore still be dropped by callers that discard a value-dead operand through that path -- e.g.fgMorphModToZero(x % 1) and theTernaryLogicunused-operand path (gtUnusedValNode). A per-site mask widen does not fix those (the global-morph extraction still drops the load); the correct fix is centralized ingtNodeHasSideEffects/gtExtractSideEffListand has a wider blast radius, so it is tracked separately.Note
This PR description was drafted by Copilot.