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

[alrz]

Fixes #57731

To facilitate subsumption-checking for list-patterns, some nodes may be inserted into the dag during construction, namely BoundDagAssignmentEvaluation and a BoundDagValueTest on the length temp. These nodes drive the general shape of the dag and help to determine unreachable states. After that, those will not play any role in subsequent phases specially in lowering which result in suboptimal codegen as demonstrated in the issue.

BoundDagAssignmentEvaluation was already skipped during DAG lowering, with this change it won't reach codegen at all.

For BoundDagValueTest we need to do additional analysis, because when the condition is actually turn out to be significant we still want to keep it in the final DAG. Otherwise we short-circuit to the "false" branch which is the default as if we never inserted such test (note that when the true branch is important we have unset the flag earlier).

Decompiled examples from the issue

  private static int Test1(int[] x)
  {
    if (x != null)
    {
      int length = x.Length;
      if (length >= 1 && x[length - 1] == 1 && x[0] == 1)
      {
        return 0;
      }
    }
    return 1;
  }

  private static int Test2(int[] x)
  {
    if (x != null)
    {
      int length = x.Length;
      if (length >= 1 && x[0] == 2 && x[length - 1] == 1)
      {
        return 0;
      }
    }
    return 3;
  }

  private static int Test3(int[] x)
  {
    if (x != null)
    {
      int length = x.Length;
      if (length >= 1)
      {
        if (x[0] == 2)
        {
          return 4;
        }
        if (x[length - 1] == 1)
        {
          return 5;
        }
      }
    }
    return 3;
  }

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

Relates to test plan #51289

[AlekseyTs]

@alrz Consider adding more details about the change to the description. What nodes are skipped, why it is the right thing to do, etc.

---

In reply to: 975716098

[alrz]

Updated. Let me know if it doesn't clearly reflect the reasoning behind the change.

---

In reply to: 976443029

[jcouv]

Done with review pass (iteration 2). Only minor comments

[jcouv]

FYI, I retargeted this PR to main branch.

---

In reply to: 978185113

[AlekseyTs]

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?

[alrz]

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.

[AlekseyTs]

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?

[AlekseyTs]

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

[alrz]

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.

[AlekseyTs]

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.

[alrz]

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.

[AlekseyTs]

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?

[alrz]

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?

[AlekseyTs]

What is your recommendation on approaching this?

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

[AlekseyTs]

Done with review pass (commit 6)

[AlekseyTs]

@alrz After an offline discussion within the team, we decided that the solution we are most comfortable with at the moment is to build a dedicated Dag for the purpose of the code gen if the Dag built for the purpose of subsumption checking contains nodes synthesized solely for the purpose of list pattern subsumption checking. That means, that users utilizing pattern matching, but not utilizing list patterns specifically, won't pay any noticeable penalty due to the list patterns feature.

[alrz]

I'll hold this off until the open issue about assumptions around slice result is resolved. At the moment it's not clear to me if we should only avoid synthesizing tests for lowering or we should bail for any non-identical indexers entirely.

[jcouv]

the open issue about assumptions around slice result is resolved.

The topic is scheduled for LDM right after New Year. Based on discussion so far, the leaning is towards having the diagnostics behave as-if Slice could never return null, but have the codegen include the null check.
A design with two DAGs (one for subsumption/exhaustiveness diagnostics, and one for codegen) makes that behavior possible. But the Slice question may imply that we need to keep both DAGs around longer, so that nullability analysis can run on the former DAG (not the codegen DAG).

[alrz]

(not the codegen DAG)

If you mean we should run the nullability analysis on the dag that "contains" the null test on the slice result (to preserve the current behavior), that'll be the codegen dag actually, however, there's a problem:

• For the first pass, we skip the null check on the slice and relate alternative indexers.
• For codegen, we keep the null check on the slice but avoid synthesizing any tests.

Neither of these graphs would be equivalent to the current one so nullability will be affected (albeit, not of the slice result if we pick the latter, rather, of any nested list subpatterns that we have determined to be related in the former dag.)

[alrz]

@AlekseyTs @jcouv

the solution we are most comfortable with at the moment is to build a dedicated Dag for the purpose of the code gen if the Dag built for the purpose of subsumption checking contains nodes synthesized solely for the purpose of list pattern subsumption checking.

While this is unlikely to change the program output, I'm wondering if this could cause a change in order of evaluation, particularly around when expressions. I'm yet to come up with an example where this is apparent but on the surface, imagine due to the implied result of the next case, we directly jump to the when clause. That would not be the case if we did not consider alternative indexers to be related - we will evaluate the pattern first and only then the when clause is evaluated. This could get worse when multiple when clauses are involved.

[alrz]

Closing in favor of #59019