Skip to content

Commit 7c58a24

Browse files
authored
JIT: Fix fgFoldCondToReturnBlock for multi-statement return blocks (dotnet#124642)
## Summary Fixes dotnet#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
1 parent 982dc8a commit 7c58a24

3 files changed

Lines changed: 79 additions & 3 deletions

File tree

src/coreclr/jit/optimizebools.cpp

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1767,12 +1767,26 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block)
17671767
return modified;
17681768
}
17691769

1770-
// Is block a BBJ_RETURN(1/0) ? (single statement)
1770+
// Is block a BBJ_RETURN(1/0) ?
17711771
auto isReturnBool = [](const BasicBlock* block, bool value) {
1772-
if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr))
1772+
if (block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr))
17731773
{
17741774
GenTree* node = block->lastStmt()->GetRootNode();
1775-
return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0);
1775+
if (!(node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0)))
1776+
{
1777+
return false;
1778+
}
1779+
// Allow preceding statements if they have no globally visible side effects
1780+
// (e.g., dead local stores left over from inlining).
1781+
for (Statement* stmt = block->firstStmt(); stmt != block->lastStmt();
1782+
stmt = stmt->GetNextStmt())
1783+
{
1784+
if (GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS(stmt->GetRootNode()->gtFlags))
1785+
{
1786+
return false;
1787+
}
1788+
}
1789+
return true;
17761790
}
17771791
return false;
17781792
};
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
// When a constant-folded operand appears after a non-constant operand in a
5+
// short-circuit && expression, inlining may leave dead local stores in the
6+
// return block. fgFoldCondToReturnBlock failed to optimize this pattern
7+
// because isReturnBool required hasSingleStmt(), which was false due to the
8+
// leftover dead stores.
9+
10+
using System;
11+
using System.Runtime.CompilerServices;
12+
using Xunit;
13+
14+
public class Runtime_123621
15+
{
16+
[MethodImpl(MethodImplOptions.NoInlining)]
17+
public static bool Hoisted(byte v)
18+
{
19+
bool isUnix = Environment.NewLine != "\r\n";
20+
return (v == 2 && isUnix);
21+
}
22+
23+
[MethodImpl(MethodImplOptions.NoInlining)]
24+
public static bool Inline_Before(byte v)
25+
{
26+
return (Environment.NewLine != "\r\n" && v == 2);
27+
}
28+
29+
[MethodImpl(MethodImplOptions.NoInlining)]
30+
public static bool Inline_After(byte v)
31+
{
32+
return (v == 2 && Environment.NewLine != "\r\n");
33+
}
34+
35+
[Fact]
36+
public static void TestEntryPoint()
37+
{
38+
bool isUnix = Environment.NewLine != "\r\n";
39+
40+
Assert.Equal(isUnix, Hoisted(2));
41+
Assert.False(Hoisted(3));
42+
43+
Assert.Equal(isUnix, Inline_Before(2));
44+
Assert.False(Inline_Before(3));
45+
46+
Assert.Equal(isUnix, Inline_After(2));
47+
Assert.False(Inline_After(3));
48+
49+
// All three methods must produce the same result.
50+
Assert.Equal(Hoisted(2), Inline_After(2));
51+
Assert.Equal(Inline_Before(2), Inline_After(2));
52+
}
53+
}
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<DebugType>PdbOnly</DebugType>
4+
<Optimize>True</Optimize>
5+
</PropertyGroup>
6+
<ItemGroup>
7+
<Compile Include="$(MSBuildProjectName).cs" />
8+
</ItemGroup>
9+
</Project>

0 commit comments

Comments
 (0)