List patterns: Recreate the decision dag for lowering by alrz · Pull Request #59019 · dotnet/roslyn

alrz · 2022-01-22T17:03:38Z

An alternative approach to #57909
Relates to test plan #51289

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

AlekseyTs · 2022-01-25T18:06:21Z

src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs

        private readonly Conversions _conversions;
        private readonly BindingDiagnosticBag _diagnostics;
        private readonly LabelSymbol _defaultLabel;
+        private readonly bool _considerAlternativeIndexers;


_considerAlternativeIndexers

Consider adding a doc comment. #Closed

I noticed the name is not entirely true - we do consider alternative indexers, we just avoid synthesizing tests. Will try something more on-point like _forLowering for an easier explanation.

AlekseyTs · 2022-01-25T18:10:15Z

src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs

                                }

-                                if (lengthValues.Any(BinaryOperatorKind.Equal, lengthValue))
+                                if (lengthValues.Any(BinaryOperatorKind.Equal, lengthValue) && _considerAlternativeIndexers)


_considerAlternativeIndexers

Consider checking this first. #Closed

AlekseyTs · 2022-01-25T18:15:07Z

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

                            return _factory.AssignmentExpression(output, access);
                        }

+                    case BoundDagAssignmentEvaluation:


case BoundDagAssignmentEvaluation:

Is default: branch not good enough? #Closed

I'd rather mention every evaluation node for an exhaustive match. The type check itself is not reachable anyways.
(I think this also address your next comment)

I'd rather mention every evaluation node for an exhaustive match. The type check itself is not reachable anyways.
(I think this also address your next comment)

How does this help to diagnose a potential failure? It feels like throw ExceptionUtilities.UnexpectedValue(evaluation) is going to be more informative by comparison to throw ExceptionUtilities.Unreachable.

I think UnexpectedValue is useful when new nodes are added and need to be covered, while this particular node already exists and with this change it must be unreachable here.
We do have some other instances of this pattern in the codebase.

I can change to throw ExceptionUtilities.UnexpectedValue(evaluation.Kind); Turned out that's more common than throwing unreachable for this propose.

Or just case BoundDagAssignmentEvaluation: default: I think that's is more aligned with your thinking.

I think UnexpectedValue is useful when new nodes are added and need to be covered, while this particular node already exists and with this change it must be unreachable here.
We do have some other instances of this pattern in the codebase.

I think ExceptionUtilities.UnexpectedValue(evaluation) is more informative and, while investigating a failure, I wouldn't really care much about whether a new node caused a failure. I definitely wouldn't try to deduce this information from the kind of exception thrown. We use ExceptionUtilities.UnexpectedValue(evaluation) for default case here and, in my opinion, there is no good reason to diverge for case BoundDagAssignmentEvaluation. We could still keep the case for it if you prefer.

Done. I think I've addressed all the comments. ptal, thanks.

AlekseyTs · 2022-01-25T18:22:47Z

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

                _factory.Syntax = test.Syntax;
                switch (test)
                {
-                    case BoundDagAssignmentEvaluation:


case BoundDagAssignmentEvaluation:

Would it make sense to keep this case with throw? #Closed

LowerEvaluation is the very next thing we call with an explicit unreachable case mentioned above.

AlekseyTs · 2022-01-25T18:31:59Z

        if (canProduceLinearSequence(node.DecisionDag.RootNode, whenTrueLabel: node.WhenTrueLabel, whenFalseLabel: node.WhenFalseLabel))

It is not obvious why it is safe to use the original Dag here. I think the implementation will be more robust if the code that is added to LowerGeneralIsPattern is added before this if instead. #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs:20 in 54f5259. [](commit_id = 54f5259, deletion_comment = False)

AlekseyTs · 2022-01-25T18:36:29Z

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

-                BoundDecisionDag decisionDag = ShareTempsIfPossibleAndEvaluateInput(
-                    node.DecisionDag, loweredSwitchGoverningExpression, result, out BoundExpression savedInputExpression);
+
+                LabelSymbol? defaultLabel = node.DefaultLabel;


defaultLabel

~~It looks like this declaration can be moved inside the if.~~ #Closed

AlekseyTs · 2022-01-25T18:40:07Z

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

-                BoundDecisionDag decisionDag = ShareTempsIfPossibleAndEvaluateInput(
-                    node.DecisionDag, inputExpression, resultBuilder, out _);
+
+                BoundDecisionDag decisionDag = node.DecisionDag;


DecisionDag

It might be useful to rename properties like this in bound nodes to something like "DecisionDagNotForCodeGeneration" #Closed

Only a single decision dag is kept in the bound tree. The other is created on the fly if needed.
(Plus DecisionDagNotForCodeGeneration is not true. Normally, we do use it for codegen.)

Only a single decision dag is kept in the bound tree. The other is created on the fly if needed.
(Plus DecisionDagNotForCodeGeneration is not true. Normally, we do use it for codegen.)

The goal is to prevent people from using this Dag for codegen without special considerations first. It does not matter much if we end up reusing the Dag at the end, the main part is that we are supposed to check if it is reusable first. An "alarm" in the name helps to force a developer (and code reviewers as well) to think about the usage from that perspective. Feel free to use a "better" name.

The goal is to prevent people from using this Dag for codegen without special considerations first. It does not matter much if we end up reusing the Dag at the end, the main part is that we are supposed to check if it is reusable first

I think I get it. How do you feel about an instance method GetDecisionDagForLowering that does this check? We can also rename the field to ReachabilityDecisionDag for explicit signal on plain usages.

How do you feel about an instance method GetDecisionDagForLowering that does this check? We can also rename the field to ReachabilityDecisionDag for explicit signal on plain usages.

I think that will address my concern. There is still a question in what type GetDecisionDagForLowering will be defined, but having a dedicated helper feels like a good idea.

AlekseyTs · 2022-01-25T18:44:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

+";
+        var verifier = CompileAndVerify(new[] { source, TestSources.Index, TestSources.Range }, parseOptions: TestOptions.RegularWithListPatterns, options: TestOptions.ReleaseDll).VerifyDiagnostics();
+        AssertEx.Multiple(
+            () => verifier.VerifyIL("X.Test1", @"


VerifyIL

Could you share result of decompiling IL to C#? #Closed

Decompiled code from ListPattern_Codegen

private static int Test1(int[] x) { if (x != null) { int num = x.Length; if (num >= 1 && x[num - 1] == 1 && x[0] == 1) { return 0; } } return 1; } private static int Test2(int[] x) { if (x != null) { int num = x.Length; if (num >= 1 && x[0] == 2 && x[num - 1] == 1) { return 0; } } return 3; } private static int Test3(int[] x) { if (x != null) { int num = x.Length; if (num >= 1) { if (x[0] == 2) { return 4; } if (x[num - 1] == 1) { return 5; } } } return 3; } private static int Test4(int[] x) { if (x != null) { int num = x.Length; if (num >= 1) { int num2 = x[0]; if (num2 == 2) { return 4; } int num3 = x[num - 1]; if (num3 == 1) { return 5; } if (num >= 2 && num2 == 6 && num3 == 7) { return 8; } } } }

AlekseyTs · 2022-01-25T18:45:41Z

Done with review pass (commit 7)

AlekseyTs · 2022-01-26T14:55:35Z

            BoundDecisionDag decisionDag = isPatternExpression.DecisionDag;

A plain usage of this property during lowering is unexpected. #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs:225 in 54f5259. [](commit_id = 54f5259, deletion_comment = False)

AlekseyTs · 2022-01-26T15:07:32Z

Done with review pass (commit 8)

alrz · 2022-01-26T18:03:26Z

src/Compilers/CSharp/Portable/BoundTree/BoundSwitchStatement.cs

    internal partial class BoundSwitchStatement : IBoundSwitchStatement
    {
        BoundNode IBoundSwitchStatement.Value => this.Expression;
        ImmutableArray<BoundStatementList> IBoundSwitchStatement.Cases => StaticCast<BoundStatementList>.From(this.SwitchSections);


❔ Not related to this PR: this interface is unused.

alrz · 2022-01-26T20:28:26Z

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

                BoundDecisionDag decisionDag = ShareTempsIfPossibleAndEvaluateInput(
-                    node.DecisionDag, loweredSwitchGoverningExpression, result, out BoundExpression savedInputExpression);
+                    node.GetDecisionDagForLowering(_factory.Compilation, out LabelSymbol? defaultLabel),
+                    loweredSwitchGoverningExpression, result, out BoundExpression savedInputExpression);


❔ The name result threw me off.. it's a builder. Should probably rename to resultBuilder here and above just like LocalRewriter_IsPatternOperator.cs

src/Compilers/CSharp/Portable/BoundTree/BoundSwitchExpression.cs

AlekseyTs

LGTM (commit 11)

AlekseyTs · 2022-01-27T15:01:27Z

@jcouv, @333fred, @dotnet/roslyn-compiler For the second review.

jcouv · 2022-02-04T16:47:07Z

I was out for a week and just got back to this. Looks good. Thanks!
PS: in general, don't hesitate to ping on PRs. It's a helpful reminder :-)

jcouv

LGTM Thanks (iteration 11)

jcouv · 2022-02-04T16:48:10Z

Fixes #57731?

alrz · 2022-02-04T18:19:48Z

Fixes #57731?

Yes, I believe so. (WorkItem marker was already added)

alrz added 2 commits January 22, 2022 20:07

WIP

480bacb

Add test

fe2722a

ghost added the Area-Compilers label Jan 22, 2022

alrz added 2 commits January 22, 2022 20:57

Revert whitespace

033d345

Adjust baseline

35b64df

alrz marked this pull request as ready for review January 22, 2022 18:23

alrz requested a review from a team as a code owner January 22, 2022 18:23

alrz requested review from AlekseyTs and jcouv January 22, 2022 18:23

alrz commented Jan 22, 2022

View reviewed changes

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

alrz mentioned this pull request Jan 22, 2022

List patterns: Skip compiler-generated dag nodes for better codegen #57909

Closed

alrz added 3 commits January 22, 2022 22:09

Revert

3328a15

Simplify

8367c29

Assert

54f5259

AlekseyTs reviewed Jan 25, 2022

View reviewed changes

PR feedback

053e22d

Move code

3e7b26a

alrz commented Jan 26, 2022

View reviewed changes

alrz added 2 commits January 26, 2022 21:52

Rename

430d27a

Fix formatting

aa5cd44

alrz commented Jan 26, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jan 26, 2022

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

alrz commented Jan 27, 2022

View reviewed changes

src/Compilers/CSharp/Portable/BoundTree/BoundSwitchExpression.cs Show resolved Hide resolved

AlekseyTs approved these changes Jan 27, 2022

View reviewed changes

jcouv self-assigned this Feb 4, 2022

jcouv approved these changes Feb 4, 2022

View reviewed changes

jcouv merged commit c229bc4 into dotnet:main Feb 4, 2022

ghost added this to the Next milestone Feb 4, 2022

jcouv mentioned this pull request Feb 4, 2022

Suboptimal code is generated for evaluation of list patterns, could be an artifact of used subsumption checking strategy #57731

Closed

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

Conversation

alrz commented Jan 22, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 25, 2022

Uh oh!

AlekseyTs commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jan 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

alrz commented Jan 22, 2022 •

edited by jcouv

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

alrz Jan 26, 2022 •

edited

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

AlekseyTs commented Jan 25, 2022 •

edited

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

AlekseyTs Jan 26, 2022 •

edited

Loading

AlekseyTs Jan 25, 2022 •

edited

Loading

AlekseyTs commented Jan 26, 2022 •

edited

Loading