Skip to content

Fix suboptimal codegen for short or patterns in is-expressions #82429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stevenelliottjr wants to merge 3 commits into
base: main
Choose a base branch
from
+262 −183
Open

Fix suboptimal codegen for short or patterns in is-expressions #82429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6dcf4e6
Fix suboptimal codegen for short `or` patterns in is-expressions (#80…
stevenelliottjr Feb 17, 2026
8e13d09
Fix inverted linear sequence skipping variable bindings in or patterns
stevenelliottjr Feb 17, 2026
c98d9c9
Address review feedback: rephrase comment, use IsEmpty, assert M2 IL,…
stevenelliottjr Feb 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 56 additions & 9 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ namespace Microsoft.CodeAnalysis.CSharp
{
internal sealed partial class LocalRewriter
{
/// <summary>
/// Maximum number of test nodes allowed in an inverted linear sequence for `or` patterns.
/// Beyond this threshold, the general DAG lowering with switch dispatch is preferred.
/// </summary>
private const int LinearSequenceMaxTests = 4;

public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node)
{
BoundDecisionDag decisionDag = node.GetDecisionDagForLowering(_factory.Compilation);
Expand All @@ -20,20 +26,29 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node
if (canProduceLinearSequence(decisionDag.RootNode, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel))
{
// If we can build a linear test sequence `(e1 && e2 && e3)` for the dag, do so.
var isPatternRewriter = new IsPatternExpressionLinearLocalRewriter(node, this);
result = isPatternRewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel);
isPatternRewriter.Free();
result = LowerIsPatternAsLinearSequence(node, decisionDag, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel);
}
else if (IsFailureNode(decisionDag.RootNode, node.WhenFalseLabel))
{
// If the given pattern always fails due to a constant input (see comments on BoundDecisionDag.SimplifyDecisionDagIfConstantInput),
// we build a linear test sequence with the whenTrue and whenFalse labels swapped and then negate the result, to keep the result a constant.
// Note that the positive case will be handled by canProduceLinearSequence above, however, we avoid to produce a full inverted linear sequence here
// because we may be able to generate better code for a sequence of `or` patterns, using a switch dispatch, for example, which is done in the general rewriter.
negated = !negated;
var isPatternRewriter = new IsPatternExpressionLinearLocalRewriter(node, this);
result = isPatternRewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel);
isPatternRewriter.Free();
result = LowerIsPatternAsLinearSequence(node, decisionDag, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel);
}
else if (canProduceLinearSequence(decisionDag.RootNode, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel, maxTests: LinearSequenceMaxTests)
&& !dagContainsBindings(decisionDag))
{
// If we can build a short linear test sequence with the labels swapped, do so and negate the result.
// This handles `or` patterns like `is '/' or '\\'` by producing an inverted AND sequence
// (e.g., `!(x != '/' && x != '\\')`) instead of the general DAG lowering which introduces
// unnecessary boolean temporaries and a SpillSequence.
// We limit this to short sequences because the general DAG lowering can use switch dispatch
// (binary search trees) which is more efficient for large `or` patterns.
// We exclude DAGs with bindings (e.g., `(B or C) and var x`) because the inverted traversal
// follows the "failure" branches, skipping BoundWhenDecisionDagNodes that contain variable
// bindings, which would leave captured variables uninitialized.
negated = !negated;
result = LowerIsPatternAsLinearSequence(node, decisionDag, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel);
}
else
{
Expand All @@ -53,11 +68,14 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node
// linear tests with a single "golden" path to the true label and all other paths leading
// to the false label? This occurs with an is-pattern expression that uses no "or" or "not"
// pattern forms.
// maxTests limits the number of BoundTestDecisionDagNode nodes in the sequence.
static bool canProduceLinearSequence(
BoundDecisionDagNode node,
LabelSymbol whenTrueLabel,
LabelSymbol whenFalseLabel)
LabelSymbol whenFalseLabel,
int maxTests = int.MaxValue)
{
int testCount = 0;
while (true)
{
switch (node)
Expand All @@ -72,6 +90,8 @@ static bool canProduceLinearSequence(
node = e.Next;
break;
case BoundTestDecisionDagNode t:
if (++testCount > maxTests)
return false;
bool falseFail = IsFailureNode(t.WhenFalse, whenFalseLabel);
if (falseFail == IsFailureNode(t.WhenTrue, whenFalseLabel))
return false;
Expand All @@ -84,6 +104,18 @@ static bool canProduceLinearSequence(
}
}

private BoundExpression LowerIsPatternAsLinearSequence(
BoundIsPatternExpression node,
BoundDecisionDag decisionDag,
LabelSymbol whenTrueLabel,
LabelSymbol whenFalseLabel)
{
var rewriter = new IsPatternExpressionLinearLocalRewriter(node, this);
var result = rewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel, whenFalseLabel);
rewriter.Free();
return result;
}

/// <summary>
/// A local rewriter for lowering an is-pattern expression. This handles the general case by lowering
/// the decision dag, and returning a "true" or "false" value as the result at the end.
Expand Down Expand Up @@ -141,6 +173,21 @@ private static bool IsFailureNode(BoundDecisionDagNode node, LabelSymbol whenFal
return node is BoundLeafDecisionDagNode l && l.Label == whenFalseLabel;
}

/// <summary>
/// Returns true if any node in the decision DAG contains variable bindings
/// (e.g., from <c>var</c> or declaration patterns).
/// </summary>
private static bool dagContainsBindings(BoundDecisionDag decisionDag)
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dagContainsBindings

Consider making this a local function in VisitIsPatternExpression. Otherwise, this name should start with a capital letter to follow our naming conventions.

{
foreach (var node in decisionDag.TopologicallySortedNodes)
{
if (node is BoundWhenDecisionDagNode { Bindings.IsEmpty: false })
return true;
}

return false;
}

private sealed class IsPatternExpressionLinearLocalRewriter : PatternLocalRewriter
{
/// <summary>
Expand Down
168 changes: 104 additions & 64 deletions src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5505,23 +5505,18 @@ public static void Main()
var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
compVerifier.VerifyIL("C.M1", """
{
// Code size 26 (0x1a)
.maxstack 1
.locals init (bool V_0)
// Code size 21 (0x15)
.maxstack 2
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0012
IL_0006: brtrue.s IL_0013
IL_0008: ldarg.0
IL_0009: isinst "long"
IL_000e: brtrue.s IL_0012
IL_0010: br.s IL_0016
IL_0012: ldc.i4.1
IL_0013: stloc.0
IL_0014: br.s IL_0018
IL_0016: ldc.i4.0
IL_0017: stloc.0
IL_0018: ldloc.0
IL_0019: ret
IL_000e: ldnull
IL_000f: cgt.un
IL_0011: br.s IL_0014
IL_0013: ldc.i4.1
IL_0014: ret
}
""");
compVerifier.VerifyIL("C.M2", """
Expand All @@ -5544,26 +5539,22 @@ .maxstack 2
compilation = CreateCompilation(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularWithPatternCombinators);
compilation.VerifyDiagnostics();
compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
compVerifier.VerifyIL("C.M1", """
compVerifier.VerifyIL("C.M1", @"
{
// Code size 24 (0x18)
.maxstack 1
.locals init (bool V_0)
// Code size 20 (0x14)
.maxstack 2
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0010
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0012
IL_0008: ldarg.0
IL_0009: isinst "long"
IL_000e: brfalse.s IL_0014
IL_0010: ldc.i4.1
IL_0011: stloc.0
IL_0012: br.s IL_0016
IL_0014: ldc.i4.0
IL_0015: stloc.0
IL_0016: ldloc.0
IL_0017: ret
IL_0009: isinst ""long""
IL_000e: ldnull
IL_000f: cgt.un
IL_0011: ret
IL_0012: ldc.i4.1
IL_0013: ret
}
""");
");
compVerifier.VerifyIL("C.M2", @"
{
// Code size 20 (0x14)
Expand Down Expand Up @@ -5605,27 +5596,18 @@ public static void Main()
var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
compVerifier.VerifyIL("C.M1", """
{
// Code size 40 (0x28)
// Code size 29 (0x1d)
.maxstack 1
.locals init (bool V_0)
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0012
IL_0006: brtrue.s IL_0017
IL_0008: ldarg.0
IL_0009: isinst "long"
IL_000e: brtrue.s IL_0012
IL_0010: br.s IL_0016
IL_0012: ldc.i4.1
IL_0013: stloc.0
IL_0014: br.s IL_0018
IL_0016: ldc.i4.0
IL_0017: stloc.0
IL_0018: ldloc.0
IL_0019: brtrue.s IL_0022
IL_001b: ldstr "False"
IL_0020: br.s IL_0027
IL_0022: ldstr "True"
IL_0027: ret
IL_000e: brtrue.s IL_0017
IL_0010: ldstr "False"
IL_0015: br.s IL_001c
IL_0017: ldstr "True"
IL_001c: ret
}
""");
compVerifier.VerifyIL("C.M2", """
Expand All @@ -5648,30 +5630,22 @@ .maxstack 1
compilation = CreateCompilation(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularWithPatternCombinators);
compilation.VerifyDiagnostics();
compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
compVerifier.VerifyIL("C.M1", """
compVerifier.VerifyIL("C.M1", @"
{
// Code size 37 (0x25)
// Code size 28 (0x1c)
.maxstack 1
.locals init (bool V_0)
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0010
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0016
IL_0008: ldarg.0
IL_0009: isinst "long"
IL_000e: brfalse.s IL_0014
IL_0010: ldc.i4.1
IL_0011: stloc.0
IL_0012: br.s IL_0016
IL_0014: ldc.i4.0
IL_0015: stloc.0
IL_0016: ldloc.0
IL_0017: brtrue.s IL_001f
IL_0019: ldstr "False"
IL_001e: ret
IL_001f: ldstr "True"
IL_0024: ret
IL_0009: isinst ""long""
IL_000e: brtrue.s IL_0016
IL_0010: ldstr ""False""
IL_0015: ret
IL_0016: ldstr ""True""
IL_001b: ret
}
""");
");
compVerifier.VerifyIL("C.M2", @"
{
// Code size 28 (0x1c)
Expand Down Expand Up @@ -6157,6 +6131,72 @@ .maxstack 2
");
}

[Fact, WorkItem(80052, "https://github.com/dotnet/roslyn/issues/80052")]
public void IsPatternDisjunct_OrPatternInAndExpression()
{
// Regression test: `or` pattern used within `&&` should not introduce unnecessary bool temporaries.
var source = @"
using System;
class C
{
static bool M1(int x, char c) => x > 0 && c is '/' or '\\';
static bool M2(int x, char c) => x > 0 && (c == '/' || c == '\\');
public static void Main()
{
Console.Write(M1(1, '/'));
Console.Write(M1(1, '\\'));
Console.Write(M1(1, 'a'));
Console.Write(M1(0, '/'));
}
}";
var compilation = CreateCompilation(source, options: TestOptions.ReleaseExe);
compilation.VerifyDiagnostics();
var expectedOutput = @"TrueTrueFalseFalse";
var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
// The `is '/' or '\\'` pattern should produce comparable IL to `c == '/' || c == '\\'`
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// The is '/' or '\\' pattern should produce comparable IL to c == '/' || c == '\\'

It feels like we should assert that M2 results in the same IL #Closed

Copy link
Copy Markdown
Contributor Author

@stevenelliottjr stevenelliottjr Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — added a VerifyIL("C.M2", ...) assertion with the same IL.

// without introducing a bool temporary or SpillSequence.
compVerifier.VerifyIL("C.M1", @"
{
// Code size 19 (0x13)
.maxstack 2
IL_0000: ldarg.0
IL_0001: ldc.i4.0
IL_0002: ble.s IL_0011
IL_0004: ldarg.1
IL_0005: ldc.i4.s 47
IL_0007: beq.s IL_000f
IL_0009: ldarg.1
IL_000a: ldc.i4.s 92
IL_000c: ceq
IL_000e: ret
IL_000f: ldc.i4.1
IL_0010: ret
IL_0011: ldc.i4.0
IL_0012: ret
}
");
compVerifier.VerifyIL("C.M2", @"
{
// Code size 19 (0x13)
.maxstack 2
IL_0000: ldarg.0
IL_0001: ldc.i4.0
IL_0002: ble.s IL_0011
IL_0004: ldarg.1
IL_0005: ldc.i4.s 47
IL_0007: beq.s IL_000f
IL_0009: ldarg.1
IL_000a: ldc.i4.s 92
IL_000c: ceq
IL_000e: ret
IL_000f: ldc.i4.1
IL_0010: ret
IL_0011: ldc.i4.0
IL_0012: ret
}
");
}

[Fact, WorkItem(46536, "https://github.com/dotnet/roslyn/issues/46536")]
public void MultiplePathsToNode_SwitchDispatch_01()
{
Expand Down
Loading
Loading