Avoid creating result temp for switch-expressions by alrz · Pull Request #69194 · dotnet/roslyn

alrz · 2023-07-24T21:27:57Z

Follow-up to #68694
Closes #44546

alrz · 2023-07-24T21:54:49Z

src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs

+            PopEvalStack();
+            _counter++;
+            EnsureOnlyEvalStack();


Same as exception filters which basically throws away the value and fence it against whatever comes after. I wonder if this is more restrictive than what's done for conditional operators? Also, I didn't understand what is being compensated for here since there's no skipped node.

alrz · 2023-07-24T22:01:49Z

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

            }

-            private BoundExpression LowerSwitchExpression(BoundConvertedSwitchExpression node)
+            private BoundExpression LowerSwitchExpressionForDebug(BoundConvertedSwitchExpression node)


From Expression Breakpoints.md

Sequence points are constrained to appear only at locations in the code where the evaluation stack is empty

I've separated the lowering for debug/release, not sure if it's possible to combine the two somehow.

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

src/Compilers/CSharp/Portable/Lowering/SpillSequenceSpiller.cs

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

alrz · 2023-07-25T16:24:25Z

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

+                    // share them as temps in the decision dag.
+                    outerVariables.AddRange(arm.Locals);
+                    switchArmsBuilder.Add(new BoundLoweredSwitchExpressionArm(
+                        arm.Syntax, arm.Locals, statements.Add(_factory.Label(arm.Label)), loweredValue));


Doing this instead of a separate label field because we want "statements" emitted before the label while "value prologue" produced in the spiller should be added afterwards. So this saves two fields and simplify lots of places especially spiller.

alrz · 2023-07-25T16:26:49Z

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

+
+partial class BoundLoweredSwitchExpression
+{
+    private partial void Validate()


It feels like a BoundNode_Validate.cs file could be used to gather all these in one place.

src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs

alrz · 2023-07-25T16:56:52Z

@AlekseyTs I'd like a high level review pass on this if you got time before I update the baseline. There's still bootstrap failures to investigate so there's definitely missing pieces that I'm not aware of (explained above). Thanks.

AlekseyTs · 2023-07-25T16:59:11Z

I'd like a high level review pass on this if you got time before I update the baseline.

Sorry, I will not be able to do that at least until September. Perhaps someone from @dotnet/roslyn-compiler will be able to pick this up before then.

RikkiGibson · 2023-08-18T21:19:17Z

When we're ready, we could also use this PR to reintroduce the is-pattern temp optimization and fix the issue we found with it.

Reproducer for the issue that caused us to revert the is-pattern temp optimization can be found in #69615.

alrz · 2024-02-27T21:33:25Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs

            CreateCompilation(source, options: TestOptions.ReleaseDll).VerifyDiagnostics().VerifyEmitDiagnostics(
-                // (9,17): error CS4013: Instance of type 'S' cannot be used inside a nested function, query expression, iterator block or async method
-                //             Q { F: { P1: true } } when await c => r, // error: cached Q.F is alive
-                Diagnostic(ErrorCode.ERR_SpecialByRefInLambda, "F").WithArguments("S").WithLocation(9, 17)
+                Diagnostic(ErrorCode.ERR_ByRefTypeAndAwait, @"o switch
+        {


How this could be gracefully reported as ERR_SpecialByRefInLambda? The test introduced in #37818

alrz · 2024-02-27T23:46:32Z

I applied the same fix (see #72273) for switch here. The first commit is already reviewed in #68694, but let me know if you'd prefer two separate PRs.

@AlekseyTs @jcouv for reviews. Thanks.

AlekseyTs · 2024-02-29T17:54:57Z

@alrz Could you please provide a meaningful description for this PR. What you are trying to achieve, and how you trying to achieve that.

alrz · 2024-02-29T18:14:19Z

This is the exact same change in #68694 but for switch-expressions. Currently the result of either expression is set to a local but that's not necessary since in both cases we will have a (label->value) jump table at the end that could be pushed onto the the stack.

AlekseyTs · 2024-03-05T15:43:56Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml


+  <Node Name="BoundLoweredSwitchExpression" Base="BoundExpression" HasValidate="true">
+    <Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
+    <Field Name="Statements" Type="ImmutableArray&lt;BoundStatement&gt;"/>


What are these statements? Either use more descriptive name, or consider adding a comment

AlekseyTs · 2024-03-05T16:15:39Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

+  </Node>
+
+  <Node Name="BoundLoweredSwitchExpressionArm" Base="BoundNode">
+    <Field Name="Locals" Type="ImmutableArray&lt;LocalSymbol&gt;"/>


I think it will be preferred to not introduce new nodes with locals that survive lowering. Can we somehow represent what we want with combination of existing nodes?

These are the same locals in BoundScope.Locals

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

Lines 114 to 116 in 37bdfe3

// Note the language scope of the locals, even though they are included for the purposes of

// lifetime analysis in the enclosing scope.

result.Add(new BoundScope(arm.Syntax, arm.Locals, statements));

BoundScope is rewritten in MethodToClassRewriter, otherwise DefineScopeLocals is used on locals. To cover the first part, I think we could use this existing node in the spiller (I suppose that's where this is relevant) and insert BoundScope instead of rewriting the node itself with assignments, if that sounds right to you I can try it in the next iteration.

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

AlekseyTs · 2024-03-05T17:05:14Z

It looks like #68694 got reverted. I think rather than bundling that with more changes, we should revert the revert and address whatever problems that PR was causing. Then we can look at more changes on top of main with that change merged.

alrz · 2024-03-05T21:52:37Z

It looks like #68694 got reverted. I think rather than bundling that with more changes, we should revert the revert and address whatever problems that PR was causing. Then we can look at more changes on top of main with that change merged.

Thanks, I opened up the follow-up PR for review #72273

# Conflicts: # src/Compilers/CSharp/Portable/BoundTree/BoundLoweredIsPatternExpression.cs # src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs # src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs # src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsPatternOperator.cs # src/Compilers/CSharp/Portable/Lowering/SpillSequenceSpiller.cs # src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs

alrz · 2024-03-28T10:23:26Z

@AlekseyTs @jcouv @dotnet/roslyn-compiler Can I get a review pass on this please? thanks.

333fred · 2024-03-29T17:26:17Z

Sorry @alrz, we're super heads-down on a couple of changes at the moment. We'll get to this when we can, hopefully sometime next week.

alrz · 2024-09-21T15:25:05Z

Leaving a note here, this is blocked because the compiler doesn't expect labels in expressions. I think that ought to be covered as part of block-expressions feature, then both of these could be lowered to that instead.

if ( x is nonlinear-pattern )

if ( { lowered-dag-statements; whenTrue: break true; whenFalse: break false; } )

return e switch  {
  p0 => 0,
  p1 => 1,
  _ => 2
};

return {
  lowered-dag-statements;
  armLabel0: break 0;
  armLabel1: break 1;
  defaultLabel: break 2;
};

With that I'd expect no difference in codegen compared to this PR.

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jul 24, 2023

alrz commented Jul 24, 2023

View reviewed changes

alrz mentioned this pull request Jul 24, 2023

Avoid creating a result temp for is-expressions #68694

Merged

alrz commented Jul 25, 2023

View reviewed changes

alrz mentioned this pull request Aug 18, 2023

Update compiler toolset to latest #69544

Closed

RikkiGibson mentioned this pull request Sep 13, 2023

ArraySegment<byte> in async method leaves Array null #69777

Closed

alrz added 2 commits February 26, 2024 19:11

Avoid creating result temp for is-expressions

1c24d15

Avoid creating result temp for switch-expressions

ef864e8

alrz mentioned this pull request Feb 26, 2024

Avoid creating result temp for is-expressions (follow-up) #72273

Merged

alrz added 2 commits February 27, 2024 21:21

Simplify

8700a94

Reset state

80b45e7

alrz force-pushed the lowered-switch branch from 18d940c to 80b45e7 Compare February 27, 2024 19:01

Revert

5f1f000

alrz force-pushed the lowered-switch branch 2 times, most recently from 33f8da2 to 32c7a8a Compare February 27, 2024 20:41

Update baseline

3cd0617

alrz force-pushed the lowered-switch branch from 32c7a8a to 3cd0617 Compare February 27, 2024 20:46

alrz added 4 commits February 28, 2024 00:53

Fix formatting

edd02ad

Join for end state

ffe56a2

Move

e0e0f87

Fixup

755afc6

alrz commented Feb 27, 2024

View reviewed changes

alrz marked this pull request as ready for review February 27, 2024 23:46

alrz requested a review from a team as a code owner February 27, 2024 23:46

AlekseyTs reviewed Mar 5, 2024

View reviewed changes

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml Show resolved Hide resolved

alrz marked this pull request as draft March 5, 2024 21:52

alrz added 2 commits March 22, 2024 20:34

Revert formatting

6d5275d

alrz marked this pull request as ready for review March 22, 2024 19:01

alrz requested review from a team, AlekseyTs and jcouv March 22, 2024 19:13

jcouv self-assigned this Mar 25, 2024

jaredpar added this to the Backlog milestone Mar 29, 2024

alrz marked this pull request as draft April 1, 2024 17:33

	// Note the language scope of the locals, even though they are included for the purposes of
	// lifetime analysis in the enclosing scope.
	result.Add(new BoundScope(arm.Syntax, arm.Locals, statements));

Conversation

alrz commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alrz Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

alrz Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alrz Jul 25, 2023 • 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!

alrz Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alrz commented Jul 25, 2023

Uh oh!

AlekseyTs commented Jul 25, 2023

Uh oh!

RikkiGibson commented Aug 18, 2023

Uh oh!

alrz Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

alrz commented Feb 27, 2024

Uh oh!

AlekseyTs commented Feb 29, 2024

Uh oh!

alrz commented Feb 29, 2024

Uh oh!

AlekseyTs Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

alrz Mar 22, 2024 • 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!

Uh oh!

AlekseyTs commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alrz commented Mar 5, 2024

Uh oh!

alrz commented Mar 28, 2024

Uh oh!

333fred commented Mar 29, 2024

Uh oh!

alrz commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alrz commented Jul 24, 2023 •

edited

Loading

alrz Jul 25, 2023 •

edited

Loading

alrz Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 5, 2024 •

edited

Loading

alrz commented Sep 21, 2024 •

edited

Loading