recursive-patterns(11): use stack spiller for switch expressions #25697

gafter · 2018-03-24T01:02:33Z

This refactoring moves the spiller between initial lowering and lambda lowering

Added some improved debug-time display for synthesized locals
Added some invariant checking regarding bound nodes.
Move invocation of the spiller into LocalRewriter.Rewrite
Add an error for when an expression tree contains a switch expression
Made the local substituter apply at the top level in the expression spiller because expressions can now contain statements (BoundSpillSequence)
Move the error ERR_ByRefTypeAndAwait to IteratorAndAsyncCaptureWalker

There are two iterations:

Most of the changes
Renaming and moving a file

I separated them to make review easier, as the rename and move makes it look like a file was deleted and one added.

This is built on top of other pending reviews, so even though there are only two new iteration here, you will see iterations from previous PRs. Only the last two matter for this PR.

@agocke @cston Can you review this please?
/cc @dotnet/roslyn-compiler

gafter · 2018-03-25T23:33:04Z

This refactoring moves the spiller between initial lowering and lambda lowering

Added some improved debug-time display for synthesized locals
Added some invariant checking regarding bound nodes.
Move invocation of the spiller into LocalRewriter.Rewrite
Add an error for when an expression tree contains a switch expression
Made the local substituter apply at the top level in the expression spiller because expressions can now contain statements (BoundSpillSequence)
Move the error ERR_ByRefTypeAndAwait to IteratorAndAsyncCaptureWalker

There are two iterations:

Most of the changes
Renaming and moving a file

I separated them to make review easier, as the rename and move makes it look like a file was deleted and one added.

This is built on top of other pending reviews, so even though there are only two new iteration here, you will see iterations from previous PRs. Only the last two matter for this PR.

@agocke @cston Can you review this please?
/cc @dotnet/roslyn-compiler
#Resolved

- Added some improved debug-time display for synthesized locals - Added some invariant checking regarding bound nodes. - Move invocation of the spiller into LocalRewriter.Rewrite - Add an error for when an expression tree contains a switch expression - Made the local substituter apply at the top level in the expression spiller because expressions can now contain statements (BoundSpillSequence) - Move the error ERR_ByRefTypeAndAwait to IteratorAndAsyncCaptureWalker - Skip broken tests due to dotnet#25702

gafter · 2018-03-26T19:42:51Z

@dotnet/roslyn-interactive I had to skip about 50 interactive tests that are producing bound trees that violate bound tree invariants (they use locals that are not declared). Please see #25702 for details.
#Resolved

…et/roslyn into rpatterns11

cston · 2018-03-27T18:25:54Z

using System;

Copyright. #Resolved

Refers to: src/Compilers/Core/Portable/Collections/IOrderedReadOnlySet.cs:1 in f7b666d. [](commit_id = f7b666d, deletion_comment = False)

cston · 2018-03-27T18:30:00Z

using System;

Alternatively, perhaps change MethodToStateMachineRewriter._hoistedVariables to OrderedSet<Symbol> and remove this interface.

In reply to: 376626943 [](ancestors = 376626943)

Refers to: src/Compilers/Core/Portable/Collections/IOrderedReadOnlySet.cs:1 in f7b666d. [](commit_id = f7b666d, deletion_comment = False)

cston · 2018-03-27T18:30:58Z

src/Compilers/Core/Portable/Symbols/RefKind.cs

-        // There is at least one kind of anlysis that cares about this distinction - async stack spilling
+        // Used internally to track `In` arguments that were specified with `In` modifier
+        // as opposed to those that were specified with no modifiers and matched `In` parameter.
+        // There is at least one kind of anlysis that cares about this distinction - hoisting


anlysis [](start = 41, length = 7)

Typo. #Resolved

cston · 2018-03-27T18:50:18Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5348,4 +5348,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="WRN_SwitchExpressionNotExhaustive_Title" xml:space="preserve">
    <value>The switch expression does not handle all possible inputs (it is not exhaustive).</value>
  </data>
+  <data name="ERR_ExpressionTreeContainsSwitchExpression" xml:space="preserve">
+    <value>An expression tree may not contain a switch-expression.</value>


switch-expression [](start = 48, length = 17)

Perhaps switch expression (no hyphen) to match the other diagnostics. #Resolved

cston · 2018-03-27T19:10:22Z

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

@@ -172,5 +194,227 @@ internal string GetDebuggerDisplay()
            }
            return result;
        }
+
+        public void CheckLocalsDefined()


CheckLocalsDefined [](start = 20, length = 18)

[Conditional("DEBUG")] #Resolved

cston · 2018-03-27T19:13:39Z

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

+                localsScanner.Free();
+            }
+
+            void AddAll(ImmutableArray<LocalSymbol> locals)


void [](start = 12, length = 4)

private for these three methods. #Resolved

cston · 2018-03-27T19:20:24Z

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

+        {
+            if (_inExpressionLambda)
+            {
+                Error(ErrorCode.ERR_ExpressionTreeContainsSwitchExpression, node);


ERR_ExpressionTreeContainsSwitchExpression [](start = 32, length = 42)

Is there a test for this code path? #Resolved

A test will be added in a forthcoming iteration.

In reply to: 177542903 [](ancestors = 177542903)

cston · 2018-03-27T20:09:38Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AwaitExpressionSpiller.cs

+            builder.AddLocals(node.Locals);
+            var value = VisitExpression(ref builder, node.Value);
+            value = Spill(builder, value);
+            return builder.Update(value);
        }

        public override BoundNode VisitAddressOfOperator(BoundAddressOfOperator node)


ic override BoundNode Vi [](start = 12, length = 24)

Why is this necessary? Consider adding a comment. #Resolved

cston · 2018-03-27T20:14:47Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AwaitExpressionSpiller.cs

+        public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatement node)
+        {
+            var oldCurrentMethod = _F.CurrentMethod;
+            _F.CurrentMethod = node.Symbol;


node.Symbo [](start = 30, length = 11)

We don't need a separate SpillSequenceSpiller instance when visiting lamba and local function bodies? #ByDesign

Correct, because things are spilled up to the statement level. Lambdas and local functions have a top-level block containing statements at this stage.

In reply to: 177557467 [](ancestors = 177557467)

cston · 2018-03-27T20:57:41Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/IteratorAndAsyncCaptureWalker.cs

                    {
-                        // CS4013: Instance of type '{0}' cannot be used inside an anonymous function, query expression, iterator block or async method
-                        diagnostics.Add(ErrorCode.ERR_SpecialByRefInLambda, syntax.Location, type);
+                        diagnostics.Add(ErrorCode.ERR_ByRefTypeAndAwait, local.Locations[0], local.Type);


ERR_ByRefTypeAndAwait [](start = 50, length = 21)

Does this correspond to the restricted type case previously in AwaitExpressionSpiller.AddLocal? If so, perhaps add Debug.Assert(local.Type.IsRestrictedType());. #Resolved

That is true by construction. There is only one line that adds to the set from which the symbol was fetched:

if (type.IsRestrictedType()) { // ... _lazyDisallowedCaptures.Add(variable, syntax); }

In reply to: 177570217 [](ancestors = 177570217)

cston · 2018-03-27T21:19:46Z

src/Compilers/CSharp/Portable/Syntax/LambdaUtilities.cs

+                case SyntaxKind.SwitchExpression:
+                case SyntaxKind.AwaitExpression:
+                    // These translate into a BoundSpillSequence, which is then translated into a block
+                    // containins temps required for spilling subexpressions. That block has the syntax of the switch


containins [](start = 23, length = 10)

Typo #Resolved

…et/roslyn into rpatterns11

gafter · 2018-03-29T21:55:00Z

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

+            // The resulting nodes will be "spilled" to move such statements to the top
+            // level (i.e. into the enclosing statement list).
+            _needsSpilling = true;
+            BoundLocal replacement = _factory.StoreToTemp(


Is it possible that this needs to be a ref temp? #Resolved

await is never an lvalue, so this temp is never a ref temp.
In general case we can have byref spills.

In a case of await, they will be later decomposed into something that can be hoisted - like {array, index} pairs or will produce an error if it is a true byref spill - like a result of byref method call.

In a case when spill is caused by expression switch or stackalloc (in the future), byref spills would work just fine since we do not need to hoist - ref temps stay ref temps.

Perhaps need a bit more care to make sure that "readonliness" matches. - I.E. if spilling an in argument or a receiver of readonly struct member, the temp must be ref readonly. Not sure if this is already handled correctly.
It may be particularly important when spilling receivers of struct/generic calls, since mismatch in "readonly" may cause different behavior for the copying of the receiver. #Resolved

The part about "readonly" is probably better handled as a separate workitem. It would be a new scenario for spiller.
In await case we decompose ref spills, so readonlyness of temps did not matter since they would not survive. #Resolved

agocke

LGTM

VSadov · 2018-03-29T22:38:06Z

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

            {
                switch (dag)
                {
-                    case BoundEvaluationPoint p:
+                    case BoundEvaluationDecisionDagNode p:


Are these types sealed? (for faster typechecks) #Resolved

The bound node generator only generates abstract or sealed types.

In reply to: 178199580 [](ancestors = 178199580)

VSadov · 2018-03-29T22:46:23Z

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

+                if (localRewriter._needsSpilling && !loweredStatement.HasErrors)
+                {
+                    // Move spill sequences to a top-level statement. This handles "lifting" await and the switch expression.
+                    // PROTOTYPE(patterns2): should this be something that the caller does? It isn't really a "local" rewrite.


I think it is better be handled by the caller, similarly to handling await in exception handling regions.

to have clear sequence of tree transformation.

to have better attribution when investigating perf impact of lowering passes. #ByDesign

I placed this here because there is more than one caller. However We could duplicate this code for each caller. I am not sure which is better, but this simplifies the job of the caller.

In reply to: 178200828 [](ancestors = 178200828)

VSadov · 2018-03-29T23:04:45Z

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

@@ -14,14 +14,14 @@

 namespace Microsoft.CodeAnalysis.CSharp
 {
-    internal sealed class AwaitExpressionSpiller : BoundTreeRewriterWithStackGuard
+    internal sealed class SpillSequenceSpiller : BoundTreeRewriterWithStackGuard


We are likely unable to handle some expressions that are not permitted in async methods - unsafe stuff and such.

I think it is a separate workitem, but we need to make sure that all or nearly all bound expresions can be spilled. #Resolved

The switch expression can appear in an unsafe method, so spilling needs to work with almost everything. If you have specific scenarios that would illustrate the issues, that would be very helpful. Please file as a fresh issue.

In reply to: 178203355 [](ancestors = 178203355)

VSadov

LGTM
I had some comments, but I think it may be better to handle them as a separate change

alrz · 2018-03-29T23:23:46Z

src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs

-        DecisionPoint,
-        WhenClause,
-        Decision,
+        DecisionDag,


Does the order of these members matters (in a sense that related nodes would be closer together for better switch optimizations)? Currently decision tree nodes come in the middle of statements. #Resolved

There are only four kinds of dag nodes, and they are rarely treated the same. I will file an issue to move them all after statements.

In reply to: 178205735 [](ancestors = 178205735)

gafter · 2018-03-30T03:25:42Z

using System;

OrderedSet is mutable. I did this to avoid exposing mutability.

In reply to: 376628270 [](ancestors = 376628270,376626943)

Refers to: src/Compilers/Core/Portable/Collections/IOrderedReadOnlySet.cs:1 in f7b666d. [](commit_id = f7b666d, deletion_comment = False)

…et/roslyn into rpatterns11

gafter added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Feature - Pattern Matching Pattern Matching labels Mar 24, 2018

gafter added this to the 16.0 milestone Mar 24, 2018

gafter self-assigned this Mar 24, 2018

gafter requested review from a team as code owners March 24, 2018 01:02

gafter changed the base branch from master to features/recursive-patterns March 24, 2018 01:02

gafter force-pushed the rpatterns11 branch from 9f934d7 to d08b628 Compare March 25, 2018 23:16

gafter changed the title ~~WIP recursive-patterns(11): use stack spiller for switch expressions~~ recursive-patterns(11): use stack spiller for switch expressions Mar 25, 2018

gafter removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 25, 2018

gafter requested review from agocke and cston March 25, 2018 23:32

gafter force-pushed the rpatterns11 branch from fc74088 to 30b8737 Compare March 26, 2018 16:33

gafter added 2 commits March 26, 2018 12:39

Move source for the spilling pass. NO OTHER CHANGES

efb9953

gafter force-pushed the rpatterns11 branch from 30b8737 to efb9953 Compare March 26, 2018 19:39

Merge branch 'features/recursive-patterns' of https://github.com/dotn…

a7121fc

…et/roslyn into rpatterns11

cston reviewed Mar 27, 2018

View reviewed changes

Merge branch 'features/recursive-patterns' of https://github.com/dotn…

d3e2e24

…et/roslyn into rpatterns11

gafter commented Mar 29, 2018

View reviewed changes

agocke approved these changes Mar 29, 2018

View reviewed changes

VSadov reviewed Mar 29, 2018

View reviewed changes

VSadov approved these changes Mar 29, 2018

View reviewed changes

alrz reviewed Mar 29, 2018

View reviewed changes

Merge branch 'features/recursive-patterns' of https://github.com/dotn…

bbb74d1

…et/roslyn into rpatterns11

gafter merged commit 952f566 into dotnet:features/recursive-patterns Mar 30, 2018

gafter added a commit to gafter/roslyn that referenced this pull request Mar 30, 2018

Address review comments in dotnet#25697

7b273cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

recursive-patterns(11): use stack spiller for switch expressions #25697

recursive-patterns(11): use stack spiller for switch expressions #25697

gafter commented Mar 24, 2018 •

edited

Loading

gafter commented Mar 25, 2018 •

edited

Loading

gafter commented Mar 26, 2018 •

edited

Loading

cston commented Mar 27, 2018 •

edited by gafter

Loading

cston commented Mar 27, 2018

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

cston Mar 27, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

cston Mar 27, 2018 •

edited by gafter

Loading

gafter Mar 29, 2018 •

edited

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

agocke left a comment

VSadov Mar 29, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

VSadov Mar 29, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

VSadov Mar 29, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

VSadov left a comment

alrz Mar 29, 2018 •

edited by gafter

Loading

gafter Mar 30, 2018

gafter commented Mar 30, 2018

recursive-patterns(11): use stack spiller for switch expressions #25697

recursive-patterns(11): use stack spiller for switch expressions #25697

Conversation

gafter commented Mar 24, 2018 • edited Loading

gafter commented Mar 25, 2018 • edited Loading

gafter commented Mar 26, 2018 • edited Loading

cston commented Mar 27, 2018 • edited by gafter Loading

cston commented Mar 27, 2018

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

cston Mar 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

VSadov Mar 29, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

VSadov Mar 29, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

VSadov Mar 29, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

VSadov Mar 29, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

VSadov Mar 29, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

alrz Mar 29, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Mar 30, 2018

Choose a reason for hiding this comment

gafter commented Mar 30, 2018

gafter commented Mar 24, 2018 •

edited

Loading

gafter commented Mar 25, 2018 •

edited

Loading

gafter commented Mar 26, 2018 •

edited

Loading

cston commented Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

cston Mar 27, 2018 •

edited by gafter

Loading

gafter Mar 29, 2018 •

edited

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

VSadov Mar 29, 2018 •

edited by gafter

Loading

alrz Mar 29, 2018 •

edited by gafter

Loading