Enable switch dispatch for is-expressions #65874

alrz · 2022-12-08T17:08:20Z

alrz · 2022-12-08T17:13:54Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs

            }
-            else if (canProduceLinearSequence(decisionDag.RootNode, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel))


The inverted canProduceLinearSequence check - used to lower negated is-expressions - was preventing us to use the general rewriter which handles switch dispatch, but that is already accounted for: when IsNegated is true, the decision dag is created off of the inner pattern.

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

Lines 58 to 60 in 85712db

bool negated = pattern.IsNegated(out var innerPattern);

BoundDecisionDag decisionDag = DecisionDagBuilder.CreateDecisionDagForIsPattern(

this.Compilation, pattern.Syntax, expression, innerPattern, whenTrueLabel: whenTrueLabel, whenFalseLabel: whenFalseLabel, diagnostics);

Also filed #65876

alrz · 2022-12-08T17:16:02Z

src/Compilers/CSharp/Test/Emit2/Semantics/PatternMatchingTests.cs

 {
-  // Code size      161 (0xa1)
+  // Code size      167 (0xa7)


The reason for this change is that we no longer choose the (inverted) linear rewriter for or patterns so the result is not a direct boolean value.

jcouv · 2022-12-08T18:55:28Z

src/Compilers/CSharp/Test/Emit2/Semantics/PatternMatchingTests5.cs

+{
+    public static bool Test(int a)
+    {
+        return (a is 1 or 2 or 3 or 4 or 5 or 6 or 7 or 8);


nit: Consider adding a similar test with non-contiguous values (should emit a switch IL instruction if they are packed enough, or a binary tree of conditions if they are further apart).

jcouv · 2022-12-08T18:56:21Z

src/Compilers/CSharp/Test/Emit2/Semantics/PatternMatchingTests5.cs

+    }
+}
+""";
+            var compilation = CompileAndVerify(source);


Please also verify execution
Comment also applies to string test below #Closed

jcouv

LGTM Thanks (iteration 3) with minor test comments

src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs

AlekseyTs · 2022-12-20T13:43:47Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs

-                var isPatternRewriter = new IsPatternExpressionLinearLocalRewriter(node, this);
-                result = isPatternRewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel);
-                isPatternRewriter.Free();
+                result = _factory.Literal(false);


result = _factory.Literal(false);

Consider adding a comment explaining what assumption we are making here and why #Closed

AlekseyTs · 2022-12-20T13:49:05Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs

-                var isPatternRewriter = new IsPatternExpressionLinearLocalRewriter(node, this);
-                result = isPatternRewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel);
-                isPatternRewriter.Free();
+                result = _factory.Literal(false);


result = _factory.Literal(false);

Is there a targeted test covering this code path? #Closed

There's two ways an is-expression can result in a constant:

if the pattern is logically irrefutable (warns - caught by linear rewriter) or impossible (error - won't reach codegen)

same as above but due to a constant input (warns for both cases)

In the latter, the linear check would catch both and produce a constant, but since we're removing the inverted one, constant false needs to be handled separately.

There's a few tests for either scenario none of which verifies codegen except IsNot_12. I'll add more in the next iteration.

alrz · 2023-01-04T22:08:55Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs

@@ -17,20 +17,22 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node
            BoundDecisionDag decisionDag = node.GetDecisionDagForLowering(_factory.Compilation);
            bool negated = node.IsNegated;
            BoundExpression result;
-            if (canProduceLinearSequence(decisionDag.RootNode, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel))
+            if (IsFailureNode(decisionDag.RootNode, node.WhenFalseLabel))


Keeping the previous logic here but limiting the inverted scenario to immediate failures to not capture an or sequence.

AlekseyTs · 2023-01-08T00:44:08Z

/azp run

azure-pipelines · 2023-01-08T00:44:24Z

Azure Pipelines successfully started running 2 pipeline(s).

AlekseyTs · 2023-01-08T01:12:52Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs

+                // 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;


negated = !negated;

Is there a targeted test covering this code path now? #Closed

That would be IsNot_18. Let me know if there's any other scenario you want to verify. Thanks.

Took me a while to understand what you were concerning about.

In the first if we check if there's a single path to the true label with all the other paths leading to false label.

In the second if we check if there's a direct path to the false label (in which case there would be no true label).

We still need to negate the result here because whenTrue and whenFalse are swapped for the linear rewriter as it expects whenTrue to be the "final" leaf node. Of course nothing about that has changed in this PR beyond limiting the scenario that leads to this code path.

AlekseyTs · 2023-01-08T01:13:55Z

Done with review pass (commit 12)

alrz · 2023-01-11T22:03:52Z

src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs

@@ -4186,7 +4186,99 @@ .maxstack  2
        [Fact()]
        public void IsWarningSwitchEmit()


This doesn't have much utility beyond comparing the result. Let me know if I should revert since large codegen tests are generally avoided.

jcouv

LGTM Thanks (iteration 12)

AlekseyTs

LGTM (commit 12)

alrz added 3 commits December 8, 2022 19:54

Don't attempt to produce inverted linear sequence

f9bfa39

Fixup tests

fbe5bdb

Add test

b9de7e5

alrz requested a review from a team as a code owner December 8, 2022 17:08

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 8, 2022

dotnet-issue-labeler bot added the Area-Compilers label Dec 8, 2022

alrz commented Dec 8, 2022

View reviewed changes

jcouv self-assigned this Dec 8, 2022

jcouv reviewed Dec 8, 2022

View reviewed changes

jcouv approved these changes Dec 8, 2022

View reviewed changes

alrz added 2 commits December 9, 2022 12:41

Skip tests

df09a7a

Add suggested test

e6ac81b

alrz force-pushed the optimize-is branch from 238631e to e6ac81b Compare December 9, 2022 18:25

Add an comparative test and link to issue

5db93b1

333fred reviewed Dec 16, 2022

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs Outdated Show resolved Hide resolved

Unskip tests

7e6726d

alrz requested a review from a team December 20, 2022 05:56

jcouv requested review from RikkiGibson and 333fred December 20, 2022 07:58

AlekseyTs reviewed Dec 20, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Dec 20, 2022

View reviewed changes

Address feedback

7876a16

alrz commented Jan 4, 2023

View reviewed changes

alrz requested a review from AlekseyTs January 4, 2023 22:09

alrz added 3 commits January 5, 2023 01:59

Remove empty line

7dc7d08

Invert if to check the common case first

ba2a4cb

Merge remote-tracking branch 'origin/main' into optimize-is

f9ceb5f

Reword

aac7c83

AlekseyTs requested a review from jcouv January 8, 2023 00:47

AlekseyTs reviewed Jan 8, 2023

View reviewed changes

alrz commented Jan 11, 2023

View reviewed changes

jcouv added this to the 17.6 milestone Jan 12, 2023

jcouv approved these changes Jan 12, 2023

View reviewed changes

AlekseyTs approved these changes Jan 12, 2023

View reviewed changes

jcouv merged commit e5ef66e into dotnet:main Jan 12, 2023

ghost modified the milestones: 17.6, Next Jan 12, 2023

allisonchou mentioned this pull request Jan 18, 2023

[Automated] PRs inserted in VS build main-33318.109 #66470

Closed

Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023

alrz mentioned this pull request Nov 27, 2024

Infallible var pattern causes "not all code paths return a value" #76059

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable switch dispatch for is-expressions #65874

Enable switch dispatch for is-expressions #65874

alrz commented Dec 8, 2022

alrz Dec 8, 2022 •

edited

Loading

alrz Dec 8, 2022

alrz Dec 8, 2022 •

edited

Loading

jcouv Dec 8, 2022

jcouv Dec 8, 2022 •

edited

Loading

jcouv left a comment

AlekseyTs Dec 20, 2022 •

edited

Loading

AlekseyTs Dec 20, 2022 •

edited

Loading

alrz Dec 21, 2022 •

edited

Loading

alrz Jan 4, 2023

AlekseyTs commented Jan 8, 2023

azure-pipelines bot commented Jan 8, 2023

AlekseyTs Jan 8, 2023 •

edited

Loading

alrz Jan 10, 2023

alrz Jan 12, 2023 •

edited

Loading

AlekseyTs commented Jan 8, 2023

alrz Jan 11, 2023

jcouv left a comment

AlekseyTs left a comment

		}
		else if (canProduceLinearSequence(decisionDag.RootNode, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel))

	bool negated = pattern.IsNegated(out var innerPattern);
	BoundDecisionDag decisionDag = DecisionDagBuilder.CreateDecisionDagForIsPattern(
	this.Compilation, pattern.Syntax, expression, innerPattern, whenTrueLabel: whenTrueLabel, whenFalseLabel: whenFalseLabel, diagnostics);

		@@ -4186,7 +4186,99 @@ .maxstack 2
		[Fact()]
		public void IsWarningSwitchEmit()

Enable switch dispatch for is-expressions #65874

Enable switch dispatch for is-expressions #65874

Conversation

alrz commented Dec 8, 2022

alrz Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

alrz Dec 8, 2022

Choose a reason for hiding this comment

alrz Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 8, 2022

Choose a reason for hiding this comment

jcouv Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

alrz Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

alrz Jan 4, 2023

Choose a reason for hiding this comment

AlekseyTs commented Jan 8, 2023

azure-pipelines bot commented Jan 8, 2023

AlekseyTs Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

alrz Jan 10, 2023

Choose a reason for hiding this comment

alrz Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 8, 2023

alrz Jan 11, 2023

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

alrz Dec 8, 2022 •

edited

Loading

alrz Dec 8, 2022 •

edited

Loading

jcouv Dec 8, 2022 •

edited

Loading

AlekseyTs Dec 20, 2022 •

edited

Loading

AlekseyTs Dec 20, 2022 •

edited

Loading

alrz Dec 21, 2022 •

edited

Loading

AlekseyTs Jan 8, 2023 •

edited

Loading

alrz Jan 12, 2023 •

edited

Loading