From ce081876422bb394b2e3acbfb0470a23ff225a5d Mon Sep 17 00:00:00 2001 From: yykkibbb Date: Fri, 20 Feb 2026 23:36:08 +0900 Subject: [PATCH 1/4] JIT: Fix fgFoldCondToReturnBlock for multi-statement return blocks (#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). Fixes #123621 --- src/coreclr/jit/optimizebools.cpp | 20 +++++-- .../JitBlue/Runtime_123621/Runtime_123621.cs | 53 +++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_123621/Runtime_123621.cs diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index f9ca5451dd56d6..fde530b11756dc 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1767,12 +1767,26 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) return modified; } - // Is block a BBJ_RETURN(1/0) ? (single statement) + // Is block a BBJ_RETURN(1/0) ? auto isReturnBool = [](const BasicBlock* block, bool value) { - if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr)) + if (block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr)) { GenTree* node = block->lastStmt()->GetRootNode(); - return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); + if (!(node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0))) + { + return false; + } + // Allow preceding statements if they have no globally visible side effects + // (e.g., dead local stores left over from inlining). + for (Statement* stmt = block->firstStmt(); stmt != block->lastStmt(); + stmt = stmt->GetNextStmt()) + { + if (GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS(stmt->GetRootNode()->gtFlags)) + { + return false; + } + } + return true; } return false; }; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_123621/Runtime_123621.cs b/src/tests/JIT/Regression/JitBlue/Runtime_123621/Runtime_123621.cs new file mode 100644 index 00000000000000..430fdd579cf8e3 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_123621/Runtime_123621.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// When a constant-folded operand appears after a non-constant operand in a +// short-circuit && expression, inlining may leave dead local stores in the +// return block. fgFoldCondToReturnBlock failed to optimize this pattern +// because isReturnBool required hasSingleStmt(), which was false due to the +// leftover dead stores. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_123621 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + public static bool Hoisted(byte v) + { + bool isUnix = Environment.NewLine != "\r\n"; + return (v == 2 && isUnix); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static bool Inline_Before(byte v) + { + return (Environment.NewLine != "\r\n" && v == 2); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static bool Inline_After(byte v) + { + return (v == 2 && Environment.NewLine != "\r\n"); + } + + [Fact] + public static void TestEntryPoint() + { + bool isUnix = Environment.NewLine != "\r\n"; + + Assert.Equal(isUnix, Hoisted(2)); + Assert.False(Hoisted(3)); + + Assert.Equal(isUnix, Inline_Before(2)); + Assert.False(Inline_Before(3)); + + Assert.Equal(isUnix, Inline_After(2)); + Assert.False(Inline_After(3)); + + // All three methods must produce the same result. + Assert.Equal(Hoisted(2), Inline_After(2)); + Assert.Equal(Inline_Before(2), Inline_After(2)); + } +} From 8a2123e16a28a1844e2177812b22f89131eac8c7 Mon Sep 17 00:00:00 2001 From: yykkibbb Date: Sat, 21 Feb 2026 00:37:22 +0900 Subject: [PATCH 2/4] Move test to JIT/opt/OptimizeBools per review feedback (#123621) This is an optimization improvement, not a regression fix, so the test belongs under JIT/opt/OptimizeBools. --- .../OptimizeBools}/Runtime_123621.cs | 0 src/tests/JIT/opt/OptimizeBools/Runtime_123621.csproj | 9 +++++++++ 2 files changed, 9 insertions(+) rename src/tests/JIT/{Regression/JitBlue/Runtime_123621 => opt/OptimizeBools}/Runtime_123621.cs (100%) create mode 100644 src/tests/JIT/opt/OptimizeBools/Runtime_123621.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_123621/Runtime_123621.cs b/src/tests/JIT/opt/OptimizeBools/Runtime_123621.cs similarity index 100% rename from src/tests/JIT/Regression/JitBlue/Runtime_123621/Runtime_123621.cs rename to src/tests/JIT/opt/OptimizeBools/Runtime_123621.cs diff --git a/src/tests/JIT/opt/OptimizeBools/Runtime_123621.csproj b/src/tests/JIT/opt/OptimizeBools/Runtime_123621.csproj new file mode 100644 index 00000000000000..b47c3e8e8d9f55 --- /dev/null +++ b/src/tests/JIT/opt/OptimizeBools/Runtime_123621.csproj @@ -0,0 +1,9 @@ + + + PdbOnly + True + + + + + From 74defdea39e7e27eab3a32ee3b2e789e7e74f9ad Mon Sep 17 00:00:00 2001 From: yykkibbb Date: Sat, 21 Feb 2026 02:20:59 +0900 Subject: [PATCH 3/4] JIT: Fix VN-based dead store removal for unused SSA defs (#123621) 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 --- src/coreclr/jit/compiler.h | 7 +++ src/coreclr/jit/optimizebools.cpp | 18 +----- src/coreclr/jit/optimizer.cpp | 101 +++++++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6d63894fa8f8ea..00594814ff718e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -293,6 +293,13 @@ class LclSsaVarDsc return m_numUses; } + void ResetUses() + { + m_numUses = 0; + m_hasPhiUse = false; + m_hasGlobalUse = false; + } + void AddUse(BasicBlock* block) { if (block != m_block) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index fde530b11756dc..0b0f0fc2ea59a8 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1769,24 +1769,10 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) // Is block a BBJ_RETURN(1/0) ? auto isReturnBool = [](const BasicBlock* block, bool value) { - if (block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr)) + if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt()) { GenTree* node = block->lastStmt()->GetRootNode(); - if (!(node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0))) - { - return false; - } - // Allow preceding statements if they have no globally visible side effects - // (e.g., dead local stores left over from inlining). - for (Statement* stmt = block->firstStmt(); stmt != block->lastStmt(); - stmt = stmt->GetNextStmt()) - { - if (GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS(stmt->GetRootNode()->gtFlags)) - { - return false; - } - } - return true; + return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); } return false; }; diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 2087f3a040a2d8..cb02e48b9aa1b4 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5727,6 +5727,51 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() bool madeChanges = false; + // Recount actual SSA uses. Earlier phases (e.g., Redundant branch opts) may + // have removed trees that referenced SSA defs without updating the use counts. + // A fresh count lets us identify stores whose values are truly never read. + for (unsigned lclNum = 0; lclNum < lvaCount; lclNum++) + { + if (!lvaInSsa(lclNum)) + { + continue; + } + + LclVarDsc* varDsc = lvaGetDesc(lclNum); + unsigned defCount = varDsc->lvPerSsaData.GetCount(); + for (unsigned i = 0; i < defCount; i++) + { + varDsc->lvPerSsaData.GetSsaDefByIndex(i)->ResetUses(); + } + } + + for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) + { + for (Statement* stmt = block->firstStmt(); stmt != nullptr; stmt = stmt->GetNextStmt()) + { + for (GenTree* node = stmt->GetTreeList(); node != nullptr; node = node->gtNext) + { + if (node->OperIsLocalRead()) + { + GenTreeLclVarCommon* lcl = node->AsLclVarCommon(); + unsigned lclNum = lcl->GetLclNum(); + if (lvaInSsa(lclNum) && lcl->HasSsaName()) + { + LclVarDsc* varDsc = lvaGetDesc(lclNum); + if (node->OperIs(GT_PHI_ARG)) + { + varDsc->GetPerSsaData(lcl->GetSsaNum())->AddPhiUse(block); + } + else + { + varDsc->GetPerSsaData(lcl->GetSsaNum())->AddUse(block); + } + } + } + } + } + } + for (unsigned lclNum = 0; lclNum < lvaCount; lclNum++) { if (!lvaInSsa(lclNum)) @@ -5749,7 +5794,7 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() continue; } - for (unsigned defIndex = 1; defIndex < defCount; defIndex++) + for (unsigned defIndex = 0; defIndex < defCount; defIndex++) { LclSsaVarDsc* defDsc = varDsc->lvPerSsaData.GetSsaDefByIndex(defIndex); GenTreeLclVarCommon* store = defDsc->GetDefNode(); @@ -5766,6 +5811,60 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() continue; } + // If this SSA def has no remaining uses, the store is dead + // regardless of the stored value. Earlier phases (e.g., + // Redundant branch opts) may have removed all references. + if (defDsc->GetNumUses() == 0) + { + JITDUMP("Removed dead store (no remaining uses):\n"); + DISPTREE(store); + + GenTree* data = store->AsLclVarCommon()->Data(); + + bool removed = false; + if ((data->gtFlags & GTF_SIDE_EFFECT) == 0) + { + // Data has no side effects; try to remove the entire statement. + BasicBlock* block = defDsc->GetBlock(); + for (Statement* stmt = block->firstStmt(); stmt != nullptr; stmt = stmt->GetNextStmt()) + { + if (stmt->GetRootNode() == store) + { + fgRemoveStmt(block, stmt); + removed = true; + break; + } + } + } + + if (!removed) + { + // Data has side effects or store is not the statement root; + // keep them via a COMMA. + // TODO-ASG: delete this hack. + GenTree* nop = gtNewNothingNode(); + data->gtNext = nop; + nop->gtPrev = data; + nop->gtNext = store; + store->gtPrev = nop; + + store->ChangeOper(GT_COMMA); + store->AsOp()->gtOp2 = nop; + store->gtType = TYP_VOID; + store->SetAllEffectsFlags(data); + gtUpdateTreeAncestorsSideEffects(store); + } + + madeChanges = true; + continue; + } + + // Value-redundancy check requires a previous def to compare against. + if (defIndex == 0) + { + continue; + } + ValueNum oldStoreValue; if ((store->gtFlags & GTF_VAR_USEASG) == 0) { From 37bc0a75911bfbf962795f01718e7ec3f0ee0284 Mon Sep 17 00:00:00 2001 From: yykkibbb Date: Sat, 21 Feb 2026 03:03:57 +0900 Subject: [PATCH 4/4] Revert VN-based dead store removal changes per review feedback (#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. --- src/coreclr/jit/compiler.h | 7 --- src/coreclr/jit/optimizebools.cpp | 18 +++++- src/coreclr/jit/optimizer.cpp | 101 +----------------------------- 3 files changed, 17 insertions(+), 109 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 00594814ff718e..6d63894fa8f8ea 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -293,13 +293,6 @@ class LclSsaVarDsc return m_numUses; } - void ResetUses() - { - m_numUses = 0; - m_hasPhiUse = false; - m_hasGlobalUse = false; - } - void AddUse(BasicBlock* block) { if (block != m_block) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 0b0f0fc2ea59a8..fde530b11756dc 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1769,10 +1769,24 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) // Is block a BBJ_RETURN(1/0) ? auto isReturnBool = [](const BasicBlock* block, bool value) { - if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt()) + if (block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr)) { GenTree* node = block->lastStmt()->GetRootNode(); - return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); + if (!(node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0))) + { + return false; + } + // Allow preceding statements if they have no globally visible side effects + // (e.g., dead local stores left over from inlining). + for (Statement* stmt = block->firstStmt(); stmt != block->lastStmt(); + stmt = stmt->GetNextStmt()) + { + if (GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS(stmt->GetRootNode()->gtFlags)) + { + return false; + } + } + return true; } return false; }; diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index cb02e48b9aa1b4..2087f3a040a2d8 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5727,51 +5727,6 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() bool madeChanges = false; - // Recount actual SSA uses. Earlier phases (e.g., Redundant branch opts) may - // have removed trees that referenced SSA defs without updating the use counts. - // A fresh count lets us identify stores whose values are truly never read. - for (unsigned lclNum = 0; lclNum < lvaCount; lclNum++) - { - if (!lvaInSsa(lclNum)) - { - continue; - } - - LclVarDsc* varDsc = lvaGetDesc(lclNum); - unsigned defCount = varDsc->lvPerSsaData.GetCount(); - for (unsigned i = 0; i < defCount; i++) - { - varDsc->lvPerSsaData.GetSsaDefByIndex(i)->ResetUses(); - } - } - - for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) - { - for (Statement* stmt = block->firstStmt(); stmt != nullptr; stmt = stmt->GetNextStmt()) - { - for (GenTree* node = stmt->GetTreeList(); node != nullptr; node = node->gtNext) - { - if (node->OperIsLocalRead()) - { - GenTreeLclVarCommon* lcl = node->AsLclVarCommon(); - unsigned lclNum = lcl->GetLclNum(); - if (lvaInSsa(lclNum) && lcl->HasSsaName()) - { - LclVarDsc* varDsc = lvaGetDesc(lclNum); - if (node->OperIs(GT_PHI_ARG)) - { - varDsc->GetPerSsaData(lcl->GetSsaNum())->AddPhiUse(block); - } - else - { - varDsc->GetPerSsaData(lcl->GetSsaNum())->AddUse(block); - } - } - } - } - } - } - for (unsigned lclNum = 0; lclNum < lvaCount; lclNum++) { if (!lvaInSsa(lclNum)) @@ -5794,7 +5749,7 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() continue; } - for (unsigned defIndex = 0; defIndex < defCount; defIndex++) + for (unsigned defIndex = 1; defIndex < defCount; defIndex++) { LclSsaVarDsc* defDsc = varDsc->lvPerSsaData.GetSsaDefByIndex(defIndex); GenTreeLclVarCommon* store = defDsc->GetDefNode(); @@ -5811,60 +5766,6 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() continue; } - // If this SSA def has no remaining uses, the store is dead - // regardless of the stored value. Earlier phases (e.g., - // Redundant branch opts) may have removed all references. - if (defDsc->GetNumUses() == 0) - { - JITDUMP("Removed dead store (no remaining uses):\n"); - DISPTREE(store); - - GenTree* data = store->AsLclVarCommon()->Data(); - - bool removed = false; - if ((data->gtFlags & GTF_SIDE_EFFECT) == 0) - { - // Data has no side effects; try to remove the entire statement. - BasicBlock* block = defDsc->GetBlock(); - for (Statement* stmt = block->firstStmt(); stmt != nullptr; stmt = stmt->GetNextStmt()) - { - if (stmt->GetRootNode() == store) - { - fgRemoveStmt(block, stmt); - removed = true; - break; - } - } - } - - if (!removed) - { - // Data has side effects or store is not the statement root; - // keep them via a COMMA. - // TODO-ASG: delete this hack. - GenTree* nop = gtNewNothingNode(); - data->gtNext = nop; - nop->gtPrev = data; - nop->gtNext = store; - store->gtPrev = nop; - - store->ChangeOper(GT_COMMA); - store->AsOp()->gtOp2 = nop; - store->gtType = TYP_VOID; - store->SetAllEffectsFlags(data); - gtUpdateTreeAncestorsSideEffects(store); - } - - madeChanges = true; - continue; - } - - // Value-redundancy check requires a previous def to compare against. - if (defIndex == 0) - { - continue; - } - ValueNum oldStoreValue; if ((store->gtFlags & GTF_VAR_USEASG) == 0) {