JIT: Fix fgFoldCondToReturnBlock for multi-statement return blocks by yykkibbb · Pull Request #124642 · dotnet/runtime

yykkibbb · 2026-02-20T14:39:47Z

Summary

When a constant-folded operand appears after a non-constant operand in a short-circuit && expression (e.g., v == 2 && Environment.NewLine != "\r\n"), callee inlining can leave dead local stores in the return block. The isReturnBool lambda in fgFoldCondToReturnBlock required hasSingleStmt(), which caused the optimization to bail out when these dead stores were present, resulting in suboptimal branching codegen.

Changes

src/coreclr/jit/optimizebools.cpp: Relax the hasSingleStmt() constraint in isReturnBool to allow preceding statements as long as they have no globally visible side effects (GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS). This enables fgFoldCondToReturnBlock to fold the conditional into a branchless return even when dead local stores from inlining remain in the block.

Before (ARM64, `Inline_After`)

cmp w0, #2 bne G_M4495_IG04mov w0, #1retG_M4495_IG04:mov w0, #0ret

After (ARM64, `Inline_After`)

cmp w0, #2 cset x0, eqret

Test plan

Added regression test Runtime_123621 covering the original issue pattern
Verified Hoisted, Inline_Before, and Inline_After all produce identical branchless codegen (cset on ARM64)
Verified existing DevDiv_168744 regression test still passes
Verified side-effect-ful blocks are correctly excluded from the optimization

…otnet#123621) When a constant-folded operand appears after a non-constant operand in a short-circuit && expression, callee inlining may leave dead local stores in the return block. The isReturnBool lambda in fgFoldCondToReturnBlock required hasSingleStmt(), causing the optimization to bail out when these dead stores were present. Relax the single-statement constraint to allow preceding statements as long as they have no globally visible side effects (e.g., dead local stores left over from inlining). Fixesdotnet#123621

dotnet-policy-service · 2026-02-20T14:41:00Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

This is an optimization improvement, not a regression fix, so the test belongs under JIT/opt/OptimizeBools.

jakobbotsch · 2026-02-20T15:53:47Z

Do you know why the dead stores aren't being removed by dead code elimination? It would probably be better to fix that if possible.

The VN-based dead store removal phase only handled value-redundant stores but had no mechanism to remove stores with zero remaining uses. After Redundant branch opts removes JTRUE references, inlining temps' dead stores were left behind, preventing fgFoldCondToReturnBlock from producing branchless codegen. - Add an SSA use recount pass to get accurate counts after earlier phases may have removed references without updating use counts - Change the inner loop to start at defIndex=0 so inlining temps' first explicit def (at m_array[0], with no implicit entry def) is considered for removal - Add a "no remaining uses" dead store removal path that removes the entire statement when data has no side effects, falling back to the COMMA approach otherwise - Revert optimizebools.cpp to the original hasSingleStmt() check, since the dead stores are now properly removed before Optimize bools

yykkibbb · 2026-02-20T17:37:09Z

@jakobbotsch Thanks so much for taking the time to review — I really appreciate the suggestion. This is my first contribution to this repo, and I'm learning as I go, so I know there may be rough edges. Your feedback has been incredibly helpful, and I want to do my best to get this right.

The VN-based dead store removal phase only handles value-redundant stores (where the new value matches the previous def). It doesn't detect stores that simply have no remaining uses — which is exactly the case here after Redundant branch opts removes the referencing JTRUEs.

I've updated the PR to fix this at the DCE level:

Added an SSA use recount pass at the start of optVNBasedDeadStoreRemoval (use counts become stale after earlier phases remove references)
Added a "no remaining uses" removal path (GetNumUses() == 0)
Changed the loop to start at defIndex = 0 so inlining temps' first explicit def is also considered
Reverted optimizebools.cpp back to the original hasSingleStmt() check

Both dead stores are now properly removed, and fgFoldCondToReturnBlock produces branchless cset codegen.

jakobbotsch · 2026-02-20T17:55:03Z

It was enough to say that this is dependent on RBO. I think this change is too invasive – I would go back to what you had before.

…t#123621) Revert the DCE-level fix as it is too invasive. The original optimizebools.cpp approach (allowing multi-statement return blocks in isReturnBool) is sufficient since the dead stores are dependent on Redundant branch opts.

yykkibbb · 2026-02-20T18:05:21Z

@jakobbotsch Thank you for the feedback — I've reverted the DCE changes and restored the original approach.

jakobbotsch · 2026-03-22T23:46:03Z

/azp run runtime-coreclr superpmi-diffs

azure-pipelines · 2026-03-22T23:46:19Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch

Thanks

…124642) ## Summary Fixes#123621 When a constant-folded operand appears **after** a non-constant operand in a short-circuit `&&` expression (e.g., `v == 2 && Environment.NewLine != "\r\n"`), callee inlining can leave dead local stores in the return block. The `isReturnBool` lambda in `fgFoldCondToReturnBlock` required `hasSingleStmt()`, which caused the optimization to bail out when these dead stores were present, resulting in suboptimal branching codegen. ### Changes - **`src/coreclr/jit/optimizebools.cpp`**: Relax the `hasSingleStmt()` constraint in `isReturnBool` to allow preceding statements as long as they have no globally visible side effects (`GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS`). This enables `fgFoldCondToReturnBlock` to fold the conditional into a branchless return even when dead local stores from inlining remain in the block. ### Before (ARM64, `Inline_After`) ```asm cmp w0, #2 bne G_M4495_IG04 mov w0, #1 ret G_M4495_IG04: mov w0, #0 ret ``` ### After (ARM64, `Inline_After`) ```asm cmp w0, #2 cset x0, eq ret ``` ## Test plan - [x] Added regression test `Runtime_123621` covering the original issue pattern - [x] Verified `Hoisted`, `Inline_Before`, and `Inline_After` all produce identical branchless codegen (`cset` on ARM64) - [x] Verified existing `DevDiv_168744` regression test still passes - [x] Verified side-effect-ful blocks are correctly excluded from the optimization

github-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 20, 2026

dotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 20, 2026

EgorBo reviewed Feb 20, 2026
View reviewed changes

Comment threadsrc/coreclr/jit/optimizebools.cpp

EgorBo reviewed Feb 20, 2026
View reviewed changes

Comment threadsrc/tests/JIT/opt/OptimizeBools/Runtime_123621.cs

Move test to JIT/opt/OptimizeBools per review feedback (dotnet#123621)
8a2123e
This is an optimization improvement, not a regression fix, so the test belongs under JIT/opt/OptimizeBools.

yykkibbb force-pushed the fix/constant-folded-operand-codegen-123621 branch from c2be40f to 8a2123eCompare February 20, 2026 15:39

yykkibbb force-pushed the fix/constant-folded-operand-codegen-123621 branch from 09d46df to 4f539bbCompare February 20, 2026 17:19

yykkibbb force-pushed the fix/constant-folded-operand-codegen-123621 branch from 4f539bb to 74defdeCompare February 20, 2026 17:21

build-analysisBot mentioned this pull request Feb 20, 2026
[android][clr] No peer certificates when executing System.Net.Http.Functional.Tests on Android emulator #124526
Closed

JulieLeeMSFT requested a review from jakobbotsch March 16, 2026 14:50

jakobbotsch approved these changes Mar 23, 2026
View reviewed changes

jakobbotsch merged commit 7c58a24 into dotnet:mainMar 23, 2026
133 of 135 checks passed

dotnet-maestroBot mentioned this pull request Mar 23, 2026
[main] Source code updates from dotnet/runtime dotnet/dotnet#5636
Merged

jakobbotsch mentioned this pull request Mar 23, 2026
JIT: Fix jit-format break #125962
Merged

github-actionsBot locked and limited conversation to collaborators Apr 22, 2026

Navigation Menu

Uh oh!

JIT: Fix fgFoldCondToReturnBlock for multi-statement return blocks - #124642

JIT: Fix fgFoldCondToReturnBlock for multi-statement return blocks#124642
jakobbotsch merged 4 commits into
dotnet:mainfrom
yykkibbb:fix/constant-folded-operand-codegen-123621

yykkibbb commented Feb 20, 2026

Uh oh!

dotnet-policy-serviceBot commented Feb 20, 2026

Uh oh!

Uh oh!

Uh oh!

jakobbotsch commented Feb 20, 2026

Uh oh!

yykkibbb commented Feb 20, 2026

Uh oh!

jakobbotsch commented Feb 20, 2026

Uh oh!

yykkibbb commented Feb 20, 2026

Uh oh!

jakobbotsch commented Mar 22, 2026

Uh oh!

azure-pipelinesBot commented Mar 22, 2026

Uh oh!

jakobbotsch left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

yykkibbb commented Feb 20, 2026

Summary

Changes

Before (ARM64, Inline_After)

After (ARM64, Inline_After)

Test plan

Uh oh!

dotnet-policy-serviceBot commented Feb 20, 2026

Uh oh!

Uh oh!

Uh oh!

jakobbotsch commented Feb 20, 2026

Uh oh!

yykkibbb commented Feb 20, 2026

Uh oh!

jakobbotsch commented Feb 20, 2026

Uh oh!

yykkibbb commented Feb 20, 2026

Uh oh!

jakobbotsch commented Mar 22, 2026

Uh oh!

azure-pipelinesBot commented Mar 22, 2026

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Before (ARM64, `Inline_After`)

After (ARM64, `Inline_After`)