Skip to content

Fix suboptimal IL for small is patterns #84961

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

Merged
jjonescz merged 6 commits into from
Aug 25, 2026
Merged

Fix suboptimal IL for small is patterns #84961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42ab28d
Fix suboptimal IL for small `is` patterns
jjonescz Aug 19, 2026
6542492
Change threshold to 3
jjonescz Aug 20, 2026
dfdb057
Fixup IL baselines
jjonescz Aug 20, 2026
1525a03
Add a comment
jjonescz Aug 20, 2026
4570a91
Improve tests
jjonescz Aug 21, 2026
478bf1a
Verify runtime async
jjonescz Aug 24, 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
55 changes: 48 additions & 7 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>
/// Benchmark results (see https://github.com/dotnet/roslyn/pull/84961) show that short patterns can be more

@RikkiGibson RikkiGibson Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be desirable to commit at least a subset of the benchmarks to src/Tools/Benchmarks/Benchmarks.csproj

@jjonescz jjonescz Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea. I think I will leave that to a follow up if that's okay.

@jjonescz jjonescz Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#85027

/// efficient when emitted as comparisons. Larger patterns can benefit from switch dispatch.
/// </summary>
private const int MaxTestsForInvertedLinearSequence = 3;

public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node)
{
BoundDecisionDag decisionDag = node.GetDecisionDagForLowering(_factory.Compilation);
Expand All @@ -20,9 +26,7 @@ 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))

@RikkiGibson RikkiGibson Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: It seems like this block could be deleted, I would expect the newly added block on line 40 to have the same effect

@jjonescz jjonescz Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This block also handles patterns with bindings which the block below does not.

@RikkiGibson RikkiGibson Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't understand this part, are you saying that a case like x is var y is handled by this?

@jjonescz jjonescz Aug 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

x is var y would be handled by the first if. This branch handles a very specific case where the input is constant and so we can determine it is false at compile time (but there can still be a binding), e.g., const int x = 2; x is 3 and int y.

{
Expand All @@ -31,9 +35,18 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node
// 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: MaxTestsForInvertedLinearSequence) &&
Comment thread
333fred marked this conversation as resolved.
!containsBindings(decisionDag))

@RikkiGibson RikkiGibson Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the reason for needing to bailout when bindings are used, but not in the positive case? I guess when we start inverting things, then, keeping track of when the binding is actually assigned gets more complicated?

@jjonescz jjonescz Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The inverted pattern could leave the bindings unassigned.

For example, consider a pattern value is (B or C) and var match. Swapping true/false labels produces value is not B && value is not C. Then we negate it, so the final lowered form is !(value is not B && value is not C). It's equivalent to the original except it is missing the binding; we would need some special logic to preserve it.

{
// If we can build a short linear test sequence with swapped labels and no variable bindings, do so and negate the result.
negated = !negated;
result = LowerIsPatternAsLinearSequence(node, decisionDag, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel);
}
else
{
Expand All @@ -56,8 +69,10 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node
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 +87,9 @@ 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 @@ -82,6 +100,29 @@ static bool canProduceLinearSequence(
}
}
}

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

return false;
}
}

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>
Expand Down
Loading