Fix suboptimal codegen for short or patterns in is-expressions#82429
Fix suboptimal codegen for short or patterns in is-expressions#82429stevenelliottjr wants to merge 2 commits intodotnet:mainfrom
Conversation
…net#80052) For short `or` patterns (up to 4 tests), use an inverted linear test sequence instead of the general DAG lowering. The general path introduces unnecessary bool temporaries and a SpillSequence, producing larger and slower IL. The inverted linear path produces clean boolean expressions (e.g., `!(x != '/' && x != '\\')`) that the IL emitter handles efficiently. For large `or` patterns, the general DAG lowering with switch dispatch (binary search trees) is still preferred.
|
@dotnet-policy-service agree |
|
@stevenelliottjr It looks like there are legitimate test failures. #Closed |
|
Apologies for the noise, @AlekseyTs. I'll investigate the test failures and get them resolved. |
The inverted linear sequence optimization for short `or` patterns followed "failure" branches, skipping BoundWhenDecisionDagNodes that contain variable bindings. This left captured variables uninitialized, causing NullReferenceException for patterns like `(B or C) and var x`. Add dagContainsBindings guard to fall back to general DAG lowering when the pattern contains variable captures. Update IL baselines for Span pattern tests to reflect the new (correct, smaller) codegen.
f37fda5 to
8e13d09
Compare
|
@AlekseyTs I've pushed a fix. The issue was that the new inverted linear sequence optimization was skipping |
Is this just an assumption, or this statement is supported by some real numbers? #Closed |
| // linear tests with a single "golden" path to the true label and all other paths leading | ||
| // to the false label? This occurs with an is-pattern expression that uses no "or" or "not" | ||
| // pattern forms. | ||
| // When maxTests is specified, the sequence is limited to at most that many test nodes. |
I guess this is just a rephrase of an existing comment in implementation #Closed |
It looks like this code is a copy of the previous block body. Consider extracting and reusing a common helper. Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs:57 in 8e13d09. [](commit_id = 8e13d09, deletion_comment = False) |
| { | ||
| foreach (var node in decisionDag.TopologicallySortedNodes) | ||
| { | ||
| if (node is BoundWhenDecisionDagNode { Bindings.Length: > 0 }) |
| compilation.VerifyDiagnostics(); | ||
| var expectedOutput = @"TrueTrueFalseFalse"; | ||
| var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput); | ||
| // The `is '/' or '\\'` pattern should produce comparable IL to `c == '/' || c == '\\'` |
|
Done with review pass (commit 2) |
Summary
Fixes #80052
Relates to #59615, #55334
For short
orpatterns (≤4 tests), the compiler now uses an inverted linear test sequence instead of the general DAG lowering. This eliminates unnecessary boolean temporaries andBoundSpillSequencewrapping that caused IL bloat.For example,
c is '/' or '\\'now produces:instead of the general DAG path which created a bool temp with true/false assignments and gotos.
Large
orpatterns (5+ tests) continue to use the general DAG lowering with switch dispatch (binary search trees), which is more efficient at scale.IL size improvements
o is int or long(Debug)o is int or long(Release)(o is int or long) ? "True" : "False"(Debug)x > 0 && c is '/' or '\\'(Release)Why this approach vs previous attempts
This problem was previously addressed in PR #68694 (reverted in #69582) and PR #72273 (reverted in #72827 due to compiler crash #72753). Both previous attempts modified the general DAG lowering to avoid the result temp, which was fragile because it interacted with exception handler scopes (
await using, try/finally), causingNullReferenceExceptioninILBuilder.BlockedBranchDestinationSlow.This PR takes a fundamentally different approach:
orpatterns to the existing, well-tested linear rewriter with swapped labelsmaxTestsparameter tocanProduceLinearSequenceto limit the optimization to short sequencesSpillSequenceand doesn't interact with exception handler scopes, avoiding the crashes that killed previous attemptsChanges
LocalRewriter_IsPatternOperator.cs: Added inverted linear sequence path for shortorpatterns; addedmaxTestsparameter tocanProduceLinearSequencePatternTests.cs: Updated IL expectations forIsPatternDisjunct_01and_02; added new testIsPatternDisjunct_OrPatternInAndExpressionfor the exact scenario from Suboptimal codegen forispattern compared to .NET 6/7 #80052Test plan
IsPatternDisjuncttests pass (5 existing + 1 new)IsWarningSwitchEmitpasses (largeorpattern still uses switch dispatch)is X or Yinsideawait using) works correctly