Fix a case where the stack spiller spills more than needed. (possibly resulting in false errors) by VSadov · Pull Request #24273 · dotnet/roslyn

VSadov · 2018-01-17T01:25:25Z

Customer scenario

Customer uses code that involves a sequence of argument expressions that includes await - most commonly it is a method call with await among arguments, bit other more complex cases exist too.

Method(a(), b(), await c() + d(), e());  // must have empty stack when `await` happens

Since stack values cannot be preserved across await suspension, compiler turns the expressions that go before the await into statement forms that store evaluation results in temporaries, which are used to rebuild the argument values after resuming from the await suspension.

var t1 = a();
var t2 = b();
var t3 = await c();              // this `await` is ok since the stack is empty
Method(t1, t2, t3 + d(), e()) ;  // <-- should be rewritten like this

The problem is that compiler spills one more argument value than needed.

var t1 = a();
var t2 = b();
var t3 = await c();
var t4 = d();                   // <-- this is redundant at best
Method(t1, t2, t3 + t4, e());

In some cases it would make no difference (when spilling is a noop, like for constants), in some cases it will result in suboptimal, but correct code, and yet in some cases when argument happen to be unspillable (like a ref-returning member) it may result in a false compile error.

Bugs this fixes

#17706

Workarounds, if any

In some cases the differences are unobservable or an ignorable small degrade. No workaround is needed.

In other cases where it results in a compile error, user may refactor the code to "spill manually" and avoid using await in call arguments..

Risk

Risk is low since it primarily affects cases where the difference is observable, which is a combination of method calls, await and ref-returning members.

Performance impact

Low perf impact because no extra allocations/no complexity changes.

Is this a regression from a previous update?

NO

Root cause analysis

The bug involves a combination of features and in most cases leads to suboptimal, but correct code.

Tests are added for cases where the difference in spilling strategy leads to error/pass distinction.
There are existing tests where the change in code quality can be observed.

How was the bug found?

Customer reported.

Test documentation updated?

N/A

The last await-containing expression in an argument triggers spilling of all argument before it. The value of the expression itself, however, should not be spilled. (there are no awaits after it) Agreesive spilling may result in substandard codegenration and/or unexpected errors.

VB does not have this particular bug and corresponding scenario works correctly. There are however other bugs beyond the scope of what is fixed in this PR.

VSadov · 2018-01-17T06:44:53Z

@dotnet/roslyn-compiler - please review.

VSadov · 2018-01-17T06:49:03Z

Note - VB has a different implementation of stack spiller and it is not affected by this particular bug. A corresponding test has been added and it works as expected.

However VB seems to have other more general issues with spilling and ref-returning members.
A bug logged to track: #24275

jaredpar · 2018-01-17T16:13:33Z

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

        {
            Debug.Assert(refKinds.IsDefault || refKinds.Length == args.Length);

+            if(args.Length == 0)


Need a space here between if and ( #Resolved

jaredpar · 2018-01-17T16:22:12Z

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


-            for (int i = lastSpill + 1; i < newList.Length; i++)
+            // the value of the last spill and everything that follows is not spilled
+            if (lastSpill < newList.Length)


The use of sideEffectsOnly in this method seems inconsistent. Depending in the content of the newList when sideEffectsOnly is true we will return an empty list, a list which is part of newList or all of newList. That intuitively seems wrong. Am I missing something subtle here? #ByDesign

Spill ensures that replacement is not sideeffecting (it is basically the "rehydrate" code). All sideeffects are moved into the spill builder. Therefore we can drop replacements when we know we only need sideeffects. - like if we are spilling a sequence. In fact some replacements are already null when coming from Spill - Spill knows when we do sideeffectsOnly and may just return null as the replacement as an easy-out when convenient.

This is essentially an optimization, but it is an important one.
If we keep unnecessary replacements, it will look like the values are spilled, survive across await and get restored - only to be unused later...

If we drop replacements right here, the spills become dead stores that do not need to be captured. Furthermore - IL Optimizer will kill dead stores and their locals, so only sideeffects will be emitted.

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

At the same time though the method name is VisitX and it returns an array of varying length. Generally I would expect VisitX style methods to return an equivalent node. Maybe it's just the name throwing me off.

I think the confusing part is that this is a bit unusual rewrite. It is not purely node-to-node. We are also filling up a builder. Parts of the input may move into the builder so it may appear that we have lost something.
Most of the VisitX methods in this rewriter are like that.

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

VSadov · 2018-01-17T23:50:35Z

@dotnet-bot test windows_release_vs-integration_prtest please

VSadov · 2018-01-18T01:50:16Z

@dotnet/roslyn-compiler - PING. Can someone take a look?

AlekseyTs · 2018-01-18T21:20:38Z

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

                var replacement = Spill(builder, newList[i], refKind, sideEffectsOnly);

                Debug.Assert(sideEffectsOnly || replacement != null);
+                Debug.Assert(!sideEffectsOnly || refKind == RefKind.None);


Debug.Assert(!sideEffectsOnly || refKind == RefKind.None); [](start = 16, length = 58)

Can we move this assert to the beginning of the function? Something like:

Debug.Assert(!sideEffectsOnly || refKinds.IsDefault); ``` #Closed

AlekseyTs · 2018-01-18T21:23:59Z

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

@@ -487,21 +492,33 @@ private ImmutableArray<BoundExpression> VisitExpressionList(
            }

            var result = ArrayBuilder<BoundExpression>.GetInstance();


GetInstance [](start = 55, length = 11)

Can we guess the required size? #Closed

newList.Length would be a fair estimate.

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

AlekseyTs

LGTM (iteration 5)

cston · 2018-01-18T22:26:14Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenRefReturnTests.vb


+        <Fact>
+        <WorkItem(17706, "https://github.com/dotnet/roslyn/issues/17706")>
+        Public Sub SpillingByrefCall_NoSpilling()


ByRef, here and in method below. #Resolved

VSadov · 2018-01-19T03:14:57Z

CC @MeiChin-Tsai for ask mode approval

AlekseyTs

LGTM

MeiChin-Tsai · 2018-01-19T18:11:16Z

approved.

VSadov · 2018-01-19T18:47:39Z

Thanks!!

VSadov added 3 commits January 16, 2018 16:22

fixed tests

9bb6ef0

added workitem attributes

eee918a

VSadov added the Area-Compilers label Jan 17, 2018

VSadov requested a review from a team as a code owner January 17, 2018 01:25

Added VB scenarios.

8d05c07

VB does not have this particular bug and corresponding scenario works correctly. There are however other bugs beyond the scope of what is fixed in this PR.

VSadov changed the title ~~[WIP] Fix stack spiller~~ Fix a case where the stack spiller spills more than needed. (possibly resulting in false errors) Jan 17, 2018

jaredpar reviewed Jan 17, 2018

View reviewed changes

PR feedback

c2faf25

jcouv added this to the 15.6 milestone Jan 18, 2018

AlekseyTs reviewed Jan 18, 2018

View reviewed changes

AlekseyTs approved these changes Jan 18, 2018

View reviewed changes

cston reviewed Jan 18, 2018

View reviewed changes

cston approved these changes Jan 18, 2018

View reviewed changes

PR feedback

1329f3e

VSadov added the Pending director approval label Jan 19, 2018

AlekseyTs approved these changes Jan 19, 2018

View reviewed changes

MeiChin-Tsai added the Approved to merge label Jan 19, 2018

VSadov removed the Pending director approval label Jan 19, 2018

VSadov merged commit 64b378e into dotnet:dev15.6.x Jan 19, 2018

VSadov mentioned this pull request Jan 19, 2018

Compiler behaves unexpectedly when compiling specific combinations of ref and await expressions #17706

Closed

		@@ -487,21 +492,33 @@ private ImmutableArray<BoundExpression> VisitExpressionList(
		}

		var result = ArrayBuilder<BoundExpression>.GetInstance();

Conversation

VSadov commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

VSadov commented Jan 17, 2018

Uh oh!

VSadov commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Jan 17, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 17, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 19, 2018 • 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!

VSadov commented Jan 17, 2018

Uh oh!

VSadov commented Jan 18, 2018

Uh oh!

AlekseyTs Jan 18, 2018 • 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!

AlekseyTs Jan 18, 2018 • 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!

VSadov Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

cston Jan 18, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Jan 19, 2018

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

MeiChin-Tsai commented Jan 19, 2018

Uh oh!

VSadov commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

VSadov commented Jan 17, 2018 •

edited

Loading

VSadov commented Jan 17, 2018 •

edited

Loading

jaredpar Jan 17, 2018 •

edited by VSadov

Loading

jaredpar Jan 17, 2018 •

edited by VSadov

Loading

VSadov Jan 19, 2018 •

edited

Loading

AlekseyTs Jan 18, 2018 •

edited

Loading

AlekseyTs Jan 18, 2018 •

edited

Loading

cston Jan 18, 2018 •

edited by VSadov

Loading