Skip to content
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

Enable switch dispatch for is-expressions #65874

Merged
merged 12 commits into from
Jan 12, 2023
Merged

Enable switch dispatch for is-expressions #65874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f9bfa39
Don't attempt to produce inverted linear sequence
alrz Dec 8, 2022
fbe5bdb
Fixup tests
alrz Dec 8, 2022
b9de7e5
Add test
alrz Dec 8, 2022
df09a7a
Skip tests
alrz Dec 9, 2022
e6ac81b
Add suggested test
alrz Dec 9, 2022
5db93b1
Add an comparative test and link to issue
alrz Dec 10, 2022
7e6726d
Unskip tests
alrz Dec 17, 2022
7876a16
Address feedback
alrz Jan 4, 2023
7dc7d08
Remove empty line
alrz Jan 4, 2023
ba2a4cb
Invert if to check the common case first
alrz Jan 5, 2023
f9ceb5f
Merge remote-tracking branch 'origin/main' into optimize-is
alrz Jan 5, 2023
aac7c83
Reword
alrz Jan 6, 2023
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
8 changes: 5 additions & 3 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 @@ -24,10 +24,12 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node
result = isPatternRewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel);
isPatternRewriter.Free();
}
else if (canProduceLinearSequence(decisionDag.RootNode, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel))
Copy link
Member Author

@alrz alrz Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

Also filed #65876

else if (IsFailureNode(decisionDag.RootNode, node.WhenFalseLabel))
{
// If we can build a linear test sequence with the whenTrue and whenFalse labels swapped, then negate the
// result. This would typically arise when the source contains `e is not pattern`.
// 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;
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

negated = !negated;

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@alrz alrz Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

var isPatternRewriter = new IsPatternExpressionLinearLocalRewriter(node, this);
result = isPatternRewriter.LowerIsPatternAsLinearTestSequence(node, decisionDag, whenTrueLabel: node.WhenFalseLabel, whenFalseLabel: node.WhenTrueLabel);
Expand Down
226 changes: 154 additions & 72 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 @@ -5517,46 +5517,83 @@ public static void Main()
compilation.VerifyDiagnostics();
var expectedOutput = @"TrueFalseTrueFalse";
var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
var code = @"
{
// Code size 21 (0x15)
.maxstack 2
IL_0000: ldarg.0
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0013
IL_0008: ldarg.0
IL_0009: isinst ""long""
IL_000e: ldnull
IL_000f: cgt.un
IL_0011: br.s IL_0014
IL_0013: ldc.i4.1
IL_0014: ret
}
";
compVerifier.VerifyIL("C.M1", code);
compVerifier.VerifyIL("C.M2", code);
compVerifier.VerifyIL("C.M1", """
{
// Code size 26 (0x1a)
.maxstack 1
.locals init (bool V_0)
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0012
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
}
""");
compVerifier.VerifyIL("C.M2", """
{
// Code size 21 (0x15)
.maxstack 2
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0013
IL_0008: ldarg.0
IL_0009: isinst "long"
IL_000e: ldnull
IL_000f: cgt.un
IL_0011: br.s IL_0014
IL_0013: ldc.i4.1
IL_0014: ret
}
""");

compilation = CreateCompilation(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularWithPatternCombinators);
compilation.VerifyDiagnostics();
compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
code = @"
{
// Code size 20 (0x14)
.maxstack 2
IL_0000: ldarg.0
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0012
IL_0008: ldarg.0
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.M1", code);
compVerifier.VerifyIL("C.M2", code);
compVerifier.VerifyIL("C.M1", """
{
// Code size 24 (0x18)
.maxstack 1
.locals init (bool V_0)
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0010
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
}
""");
compVerifier.VerifyIL("C.M2", @"
{
// Code size 20 (0x14)
.maxstack 2
IL_0000: ldarg.0
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0012
IL_0008: ldarg.0
IL_0009: isinst ""long""
IL_000e: ldnull
IL_000f: cgt.un
IL_0011: ret
IL_0012: ldc.i4.1
IL_0013: ret
}
");
}

[Fact]
Expand All @@ -5580,46 +5617,91 @@ public static void Main()
compilation.VerifyDiagnostics();
var expectedOutput = @"TrueFalseTrueFalse";
var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
var code = @"
{
// Code size 29 (0x1d)
.maxstack 1
IL_0000: ldarg.0
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0017
IL_0008: ldarg.0
IL_0009: isinst ""long""
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.M1", code);
compVerifier.VerifyIL("C.M2", code);
compVerifier.VerifyIL("C.M1", """
{
// Code size 40 (0x28)
.maxstack 1
.locals init (bool V_0)
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0012
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
}
""");
compVerifier.VerifyIL("C.M2", """
{
// Code size 29 (0x1d)
.maxstack 1
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0017
IL_0008: ldarg.0
IL_0009: isinst "long"
IL_000e: brtrue.s IL_0017
IL_0010: ldstr "False"
IL_0015: br.s IL_001c
IL_0017: ldstr "True"
IL_001c: ret
}
""");

compilation = CreateCompilation(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularWithPatternCombinators);
compilation.VerifyDiagnostics();
compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);
code = @"
{
// Code size 28 (0x1c)
.maxstack 1
IL_0000: ldarg.0
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0016
IL_0008: ldarg.0
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.M1", code);
compVerifier.VerifyIL("C.M2", code);
compVerifier.VerifyIL("C.M1", """
{
// Code size 37 (0x25)
.maxstack 1
.locals init (bool V_0)
IL_0000: ldarg.0
IL_0001: isinst "int"
IL_0006: brtrue.s IL_0010
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
}
""");
compVerifier.VerifyIL("C.M2", @"
{
// Code size 28 (0x1c)
.maxstack 1
IL_0000: ldarg.0
IL_0001: isinst ""int""
IL_0006: brtrue.s IL_0016
IL_0008: ldarg.0
IL_0009: isinst ""long""
IL_000e: brtrue.s IL_0016
IL_0010: ldstr ""False""
IL_0015: ret
IL_0016: ldstr ""True""
IL_001b: ret
}
");
}

[Fact]
Expand Down
Loading