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

Jump to bottom

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1704778
Skip compiler-generated dag nodes
alrz Nov 21, 2021
04cd111
Extract method
alrz Nov 21, 2021
cda1fed
PR Feedback
alrz Nov 23, 2021
4a02053
Merge remote-tracking branch 'origin/main' into list-patterns-simp
alrz Nov 25, 2021
2231d67
Fixup
alrz Nov 25, 2021
b008d87
Typo
alrz Nov 25, 2021
5794267
Do not depend on WasCompilerGenerated flag
alrz Dec 2, 2021
669917a
Simplify
alrz Dec 2, 2021
6f5536a
Merge remote-tracking branch 'origin/main' into list-patterns-simp
alrz Dec 8, 2021
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
61 changes: 41 additions & 20 deletions src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ private BoundDecisionDag MakeBoundDecisionDag(SyntaxNode syntax, ImmutableArray<

var rootDecisionDagNode = decisionDag.RootNode.Dag;
RoslynDebug.Assert(rootDecisionDagNode != null);
var boundDecisionDag = new BoundDecisionDag(rootDecisionDagNode.Syntax, rootDecisionDagNode);
var boundDecisionDag = new BoundDecisionDag(rootDecisionDagNode.Syntax, rootDecisionDagNode).RemoveCompilerGeneratedNodes();
alrz marked this conversation as resolved.
Show resolved Hide resolved
#if DEBUG
// Note that this uses the custom equality in `BoundDagEvaluation`
// to make "equivalent" evaluation nodes share the same ID.
Expand Down Expand Up @@ -873,19 +873,26 @@ DagState uniqifyState(ImmutableArray<StateForCase> cases, ImmutableDictionary<Bo
break;
case BoundDagTest d:
bool foundExplicitNullTest = false;
bool foundExplicitValueTest = false;
SplitCases(state, d,
out ImmutableArray<StateForCase> whenTrueDecisions,
out ImmutableArray<StateForCase> whenFalseDecisions,
out ImmutableDictionary<BoundDagTemp, IValueSet> whenTrueValues,
out ImmutableDictionary<BoundDagTemp, IValueSet> whenFalseValues,
ref foundExplicitNullTest);
ref foundExplicitNullTest, ref foundExplicitValueTest);
state.TrueBranch = uniqifyState(whenTrueDecisions, whenTrueValues);
state.FalseBranch = uniqifyState(whenFalseDecisions, whenFalseValues);
if (foundExplicitNullTest && d is BoundDagNonNullTest { IsExplicitTest: false } t)
{
// Turn an "implicit" non-null test into an explicit one
state.SelectedTest = new BoundDagNonNullTest(t.Syntax, isExplicitTest: true, t.Input, t.HasErrors);
}
else if (foundExplicitValueTest && d.WasCompilerGenerated && d is BoundDagValueTest v)
{
// We're going to remove compiler-generated nodes from dag right after consturction.
alrz marked this conversation as resolved.
Show resolved Hide resolved
// If we have found an explicit value test we need to unset the flag to preserve it.
state.SelectedTest = new BoundDagValueTest(v.Syntax, v.Value, v.Input, v.HasErrors);
Copy link
Contributor

Choose a reason for hiding this comment

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

state.SelectedTest = new BoundDagValueTest(v.Syntax, v.Value, v.Input, v.HasErrors);

I am assuming we have tests that confirm significance of this operation. Could you provide an example of an affected scenario and how the decompiled code looks like for it?

Copy link
Member Author

@alrz alrz Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The compiler-generated length test (L=1) is merged with the length test from the second pattern.

           switch (a)
           {
               case [.., 1]:
               case [2]:
                   return 0;
           }
            if (a != null)
            {
                int length = a.Length;
                if (length >= 1 && (a[length - 1] == 1 || (length == 1 && a[0] == 2)))
                {
                    return 0;
                }
            }

Not doing so would eliminate the whenTrue branch and cause a false subsumption error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not doing so would eliminate the whenTrue branch and cause a false subsumption error.

I thought WasCompilerGenerated flag didn't have any impact on subsumption checking. Is this incorrect?

Copy link
Contributor

@AlekseyTs AlekseyTs Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you think of a scenario when leaving this node won't be useful at the end? For example, when length is explicitly checked. Something like:

switch (a)
{
    case [.., 1]:
    case [2, ..] and {Length: <3 or > 5}:
        return 0;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't, but since we're removing nodes based on it, it should not be set where it's not supposed to.
I'm gonna go ahead and make the change to not depend on WasCompilerGenerated. I think that'll make all this more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can avoid duplicated nodes. Ignoring unused evaluations, what other improvements you think could be made to this code?

It is quite possible that the scenario doesn't lead to the duplication because one of the branches is optimized away. Basically we determined that either TrueBranch or FalseBranch is definitely false. However, I don't see anything in the implementation that would guarantee that we never end up with two branches alive.

To clarify, I am not looking for ways to improve the code. I am looking for a definitive proof that the code is "better" than the one we would produce without the list pattern subsumption checks in place. This means that we have to compare two code gen strategies and see that the one with artifacts is definitely better. Perhaps even to the point that we would want to implement it if the subsumption checking wouldn't give it to us for free. If there is no proof like that, then I think that it is better to generate code from a Dug that doesn't have any artifact. Instead of marking things and then hoping they can be removed safely and hoping for the best if they couldn't be removed.

we might as well construct a new dag dedicated to lowering.

This is definitely an option if we cannot find a way to achieve the same in some "incremental" fashion.

Copy link
Member Author

@alrz alrz Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am looking for a definitive proof that the code is "better" than the one we would produce without the list pattern subsumption checks in place.

As you mentioned, the current dag can assume the result of tests in certain branches depending on the length value. One possible definition of "better code" would be "fewer number of tests before we jump to a leaf node" and by this definition we're emitting a better code.

I don't see anything in the implementation that would guarantee that we never end up with two branches alive.

I think the fact that we can't optimally handle such branches is a general issue with the existing pattern-matching machinery. We use a simple left-to-right heuristic which may not result in optimal code with certain trees. See #29033.

Copy link
Contributor

@AlekseyTs AlekseyTs Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

One possible definition of "better code" would be "fewer number of tests before we jump to a leaf node" and by this definition we're emitting a better code.

This is a theory or a hope. This must be proven.

I think the fact that we can't optimally handle such branches is a general issue with the existing pattern-matching machinery. We use a simple left-to-right heuristic which may not result in optimal code with certain trees. See #29033.

Again, I am not going for the most optimal code gen. I simply would like to see a confirmation that there is a good reason to keep any artifacts of the list pattern subsumption checks in the generated code. We are complicating implementation, making an assumption that we are able to accurately detect "unremovable" artifacts. We are also complicating the future maintenance of the machinery, we have to make sure that future changes do not invalidate the assumption, etc. All this comes with an additional risk and cost. Why do we want to take it? Is there a good reason for that?

Copy link
Member Author

@alrz alrz Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Simply removing all the artifacts won't work as they're practically part of the input now and the resulting dag depends on them to occur at some point during execution. We can, however, short-circuit some of them that aren't crucial to the result.

I think we have two options moving forward:

  1. Move the logic to detect unavoidable artifacts to a later step maybe based on dag nodes themselves instead of looking for a "matching source test" which I take it is vaguely defined.
  2. Re-compute the dag for lowering. Maybe we can reuse some of the nodes but to my understanding, we can't do it as a "modification" to the existing graph. The two could be very different in shape to be able to "translate back" to the original.

What is your recommendation on approaching this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is your recommendation on approaching this?

Thank you, @alrz. We will discuss this internally and will get back to you early next week.

}
break;
case var n:
throw ExceptionUtilities.UnexpectedValue(n.Kind);
Expand Down Expand Up @@ -975,7 +982,7 @@ BoundDecisionDagNode finalState(SyntaxNode syntax, LabelSymbol label, ImmutableA
BoundDecisionDagNode? next = state.TrueBranch!.Dag;
RoslynDebug.Assert(next is { });
RoslynDebug.Assert(state.FalseBranch == null);
state.Dag = uniqifyDagNode(new BoundEvaluationDecisionDagNode(e.Syntax, e, next));
state.Dag = uniqifyDagNode(new BoundEvaluationDecisionDagNode(e.Syntax, e, next) { WasCompilerGenerated = e.WasCompilerGenerated });
}
break;
case BoundDagTest d:
Expand All @@ -984,7 +991,7 @@ BoundDecisionDagNode finalState(SyntaxNode syntax, LabelSymbol label, ImmutableA
BoundDecisionDagNode? whenFalse = state.FalseBranch!.Dag;
RoslynDebug.Assert(whenTrue is { });
RoslynDebug.Assert(whenFalse is { });
state.Dag = uniqifyDagNode(new BoundTestDecisionDagNode(d.Syntax, d, whenTrue, whenFalse));
state.Dag = uniqifyDagNode(new BoundTestDecisionDagNode(d.Syntax, d, whenTrue, whenFalse) { WasCompilerGenerated = d.WasCompilerGenerated });
}
break;
case var n:
Expand All @@ -1004,9 +1011,10 @@ private void SplitCase(
IValueSet? whenFalseValues,
out StateForCase whenTrue,
out StateForCase whenFalse,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
stateForCase.RemainingTests.Filter(this, test, state, whenTrueValues, whenFalseValues, out Tests whenTrueTests, out Tests whenFalseTests, ref foundExplicitNullTest);
stateForCase.RemainingTests.Filter(this, test, state, whenTrueValues, whenFalseValues, out Tests whenTrueTests, out Tests whenFalseTests, ref foundExplicitNullTest, ref foundExplicitValueTest);
whenTrue = stateForCase.WithRemainingTests(whenTrueTests);
whenFalse = stateForCase.WithRemainingTests(whenFalseTests);
}
Expand All @@ -1018,7 +1026,8 @@ private void SplitCases(
out ImmutableArray<StateForCase> whenFalse,
out ImmutableDictionary<BoundDagTemp, IValueSet> whenTrueValues,
out ImmutableDictionary<BoundDagTemp, IValueSet> whenFalseValues,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
ImmutableArray<StateForCase> cases = state.Cases;
var whenTrueBuilder = ArrayBuilder<StateForCase>.GetInstance(cases.Length);
Expand All @@ -1036,7 +1045,8 @@ private void SplitCases(
SplitCase(
state, stateForCase, test,
whenTrueValuesOpt, whenFalseValuesOpt,
out var whenTrueState, out var whenFalseState, ref foundExplicitNullTest);
out var whenTrueState, out var whenFalseState,
ref foundExplicitNullTest, ref foundExplicitValueTest);
// whenTrueState.IsImpossible occurs when Split results in a state for a given case where the case has been ruled
// out (because its test has failed). If not whenTruePossible, we don't want to add anything to the state. In
// either case, we do not want to add the current case to the state.
Expand Down Expand Up @@ -1178,7 +1188,8 @@ private void CheckConsistentDecision(
out bool falseTestPermitsTrueOther,
out bool trueTestImpliesTrueOther,
out bool falseTestImpliesTrueOther,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
// innocent until proven guilty
trueTestPermitsTrueOther = true;
Expand Down Expand Up @@ -1280,6 +1291,8 @@ private void CheckConsistentDecision(
out trueTestPermitsTrueOther, out falseTestPermitsTrueOther, out trueTestImpliesTrueOther, out falseTestImpliesTrueOther);
break;
case BoundDagValueTest v2:
if (!v2.WasCompilerGenerated)
foundExplicitValueTest = true;
handleRelationWithValue(BinaryOperatorKind.Equal, v2.Value,
out trueTestPermitsTrueOther, out falseTestPermitsTrueOther, out trueTestImpliesTrueOther, out falseTestImpliesTrueOther);
break;
Expand Down Expand Up @@ -1385,7 +1398,7 @@ other is not (BoundDagNonNullTest or BoundDagExplicitNullTest) &&
// At this point, we have determined that two non-identical inputs refer to the same element.
// We represent this correspondence with an assignment node in order to merge the remaining values.
// If tests are related unconditionally, we won't need to do so as the remaining values are updated right away.
relationEffect = new Tests.One(new BoundDagAssignmentEvaluation(syntax, target: other.Input, input: test.Input));
relationEffect = new Tests.One(new BoundDagAssignmentEvaluation(syntax, target: other.Input, input: test.Input) { WasCompilerGenerated = true });
}
return true;

Expand Down Expand Up @@ -1429,7 +1442,8 @@ other is not (BoundDagNonNullTest or BoundDagExplicitNullTest) &&
if (lengthValues.Any(BinaryOperatorKind.Equal, lengthValue))
{
// Otherwise, we add a test to make the result conditional on the length value.
(conditions ??= ArrayBuilder<Tests>.GetInstance()).Add(new Tests.One(new BoundDagValueTest(syntax, ConstantValue.Create(lengthValue), s1LengthTemp)));
var lengthValueTest = new BoundDagValueTest(syntax, ConstantValue.Create(lengthValue), s1LengthTemp) { WasCompilerGenerated = true };
(conditions ??= ArrayBuilder<Tests>.GetInstance()).Add(new Tests.One(lengthValueTest));
continue;
}
}
Expand Down Expand Up @@ -1897,7 +1911,8 @@ public abstract void Filter(
IValueSet? whenFalseValues,
out Tests whenTrue,
out Tests whenFalse,
ref bool foundExplicitNullTest);
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest);
public virtual BoundDagTest ComputeSelectedTest() => throw ExceptionUtilities.Unreachable;
public virtual Tests RemoveEvaluation(BoundDagEvaluation e) => this;
/// <summary>
Expand All @@ -1921,7 +1936,8 @@ public override void Filter(
IValueSet? whenFalseValues,
out Tests whenTrue,
out Tests whenFalse,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
whenTrue = whenFalse = this;
}
Expand All @@ -1942,7 +1958,8 @@ public override void Filter(
IValueSet? whenFalseValues,
out Tests whenTrue,
out Tests whenFalse,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
whenTrue = whenFalse = this;
}
Expand All @@ -1966,7 +1983,8 @@ public override void Filter(
IValueSet? whenFalseValues,
out Tests whenTrue,
out Tests whenFalse,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
SyntaxNode syntax = test.Syntax;
BoundDagTest other = this.Test;
Expand All @@ -1991,7 +2009,8 @@ public override void Filter(
falseTestPermitsTrueOther: out bool falseDecisionPermitsTrueOther,
trueTestImpliesTrueOther: out bool trueDecisionImpliesTrueOther,
falseTestImpliesTrueOther: out bool falseDecisionImpliesTrueOther,
foundExplicitNullTest: ref foundExplicitNullTest);
foundExplicitNullTest: ref foundExplicitNullTest,
foundExplicitValueTest: ref foundExplicitValueTest);

Debug.Assert(relationEffect is True or One(BoundDagAssignmentEvaluation));

Expand Down Expand Up @@ -2133,9 +2152,10 @@ public override void Filter(
IValueSet? whenFalseValues,
out Tests whenTrue,
out Tests whenFalse,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
Negated.Filter(builder, test, state, whenTrueValues, whenFalseValues, out var whenTestTrue, out var whenTestFalse, ref foundExplicitNullTest);
Negated.Filter(builder, test, state, whenTrueValues, whenFalseValues, out var whenTestTrue, out var whenTestFalse, ref foundExplicitNullTest, ref foundExplicitValueTest);
whenTrue = Not.Create(whenTestTrue);
whenFalse = Not.Create(whenTestFalse);
}
Expand All @@ -2160,13 +2180,14 @@ public sealed override void Filter(
IValueSet? whenFalseValues,
out Tests whenTrue,
out Tests whenFalse,
ref bool foundExplicitNullTest)
ref bool foundExplicitNullTest,
ref bool foundExplicitValueTest)
{
var trueBuilder = ArrayBuilder<Tests>.GetInstance(RemainingTests.Length);
var falseBuilder = ArrayBuilder<Tests>.GetInstance(RemainingTests.Length);
foreach (var other in RemainingTests)
{
other.Filter(builder, test, state, whenTrueValues, whenFalseValues, out Tests oneTrue, out Tests oneFalse, ref foundExplicitNullTest);
other.Filter(builder, test, state, whenTrueValues, whenFalseValues, out Tests oneTrue, out Tests oneFalse, ref foundExplicitNullTest, ref foundExplicitValueTest);
trueBuilder.Add(oneTrue);
falseBuilder.Add(oneFalse);
}
Expand Down
Loading