Avoid using a ref local for array element l-value #24070

cston · 2018-01-05T22:34:52Z

Customer scenario

Using a co-variant array element as an l-value results in ArrayTypeMismatchException.

object[] a = new string[] { "" };
a[0] += "";

Bugs this fixes

547533

Workarounds, if any

Copy array element.

var s = a[0];
s += "";
a[0] = s;

Risk

Low: Removes an optimization for array element l-values for compound assignment (C# and VB) and passing by reference (VB).

Performance impact

Low: Removes an optimization for array element l-values for compound assignment (C# and VB) and passing by reference (VB).

Is this a regression from a previous update?

Regression from Visual Studio 2013.

How was the bug found?

Customer reported.

cston · 2018-01-05T22:35:30Z

@dotnet/roslyn-compiler, @AlekseyTs, @VSadov, please review.

AlekseyTs · 2018-01-06T00:08:35Z

@cston It looks like a bunch of tests require an IL baseline change. #Closed

AlekseyTs · 2018-01-06T00:17:07Z

src/Compilers/VisualBasic/Portable/Lowering/UseTwiceRewriter.vb

@@ -1,5 +1,7 @@
 ' Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.

+#Const DONT_USE_BYREF_LOCALS_FOR_USE_TWICE = True


I believe this affects more scenarios than we would like to. We only need to change behavior of UseTwiceArrayAccess method. #Closed

VSadov · 2018-01-06T00:34:18Z

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

@@ -507,17 +507,7 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
                    break;

                case BoundKind.ArrayAccess:
-                    if (isDynamicAssignment)


Just removing this seems pretty heavy.

We should do this only when element type is an unsealed reference type.
We definitely do not want to do this with struct arrays or we might degrade fairly common patterns like intArray[i]++

Do we have a test that generates / diffs IL to verify the intArray[i]++ case maintains the current code generation?

Since this fix addresses such a corner case, I think the right expectation is that most or none of the existing tests need to change.
And there should be a lot of existing tests doing intArray[i]++ or similar.

It looks like it is the case after the last commit.

AlekseyTs · 2018-01-06T01:01:57Z

It looks like we need to make similar changes to WithExpressionRewriter in VB. #Closed

AlekseyTs · 2018-01-06T01:05:01Z

Done with review pass (iteration 2). #Closed

cston · 2018-01-08T21:19:31Z

WithExpressionRewriter.CaptureInAByRefTemp is not called for co-variant arrays. Added tests for With.

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

agocke · 2018-01-08T22:07:14Z

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

+                            //   *tmp = *L op R;
+                            // where T is the type of L.
+                            // 
+                            // If L is an array access, the assignment is dynamic, the compile-time of the array is dynamic[] 


This paragraph doesn't mention covariant array access. Should it?

agocke · 2018-01-08T22:26:37Z

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

@@ -603,6 +604,11 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
            return variableTemp;
        }

+        private static bool IsInvariantArray(TypeSymbol type)
+        {
+            return (type as ArrayTypeSymbol)?.ElementType?.IsSealed == true;


What does this return for T where T : struct?

We can treat T where T : struct arrays as nonvariant.

I think IsSealed returns false for type parameters regardless of constraints, so it may need to be checked separately. Or not at all - it would be a rare and hardly testable scenario.

Yes, IsSealed returns false for type parameters (and IsMustInherit returns False in VB), regardless of constraints. I've added tests for Structure and reference type constraints in VB but I'm not able to hit IsInvariantArray with an array of type parameters in C#. I've left the implementations of IsInvariantArray as is, returning false for type parameters.

VSadov · 2018-01-08T23:06:23Z

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

@@ -603,6 +604,11 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
            return variableTemp;
        }

+        private static bool IsInvariantArray(TypeSymbol type)


Nit: CLR spec uses "non-variant" term for this.

AlekseyTs · 2018-01-08T23:09:16Z

@cston It looks like there are legitimate test failures. #Closed

VSadov · 2018-01-08T23:23:58Z

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

+    }
+    static void G(object[] a, string s)
+    {
+        a[0] += s;


Perhaps one more test where the index is a nonconstant expression like a function call - to see two temps.

VSadov

LGTM

cston · 2018-01-09T19:14:22Z

@dotnet-bot test windows_release_vs-integration_prtest

cston · 2018-01-09T19:15:31Z

@AlekseyTs, @jaredpar, @agocke, all feedback has been addressed.

AlekseyTs · 2018-01-09T19:17:08Z

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

@@ -603,6 +604,11 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
            return variableTemp;
        }

+        private static bool IsInvariantArray(TypeSymbol type)
+        {
+            return (type as ArrayTypeSymbol)?.ElementType?.IsSealed == true;


? [](start = 57, length = 1)

It feels like the second conditional access is redundant. #Closed

AlekseyTs · 2018-01-09T19:24:01Z

src/Compilers/VisualBasic/Portable/Lowering/UseTwiceRewriter.vb

@@ -208,7 +208,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic

            Debug.Assert(node.IsLValue)

-#If DONT_USE_BYREF_LOCALS_FOR_USE_TWICE Then


It is not clear why is this conditional compilation got removed from this method. I would expect it to be adjusted to ensure that UseTwiceLValue is still not called when DONT_USE_BYREF_LOCALS_FOR_USE_TWICE is defined. #Closed

Added #If.

AlekseyTs · 2018-01-09T19:24:54Z

src/Compilers/VisualBasic/Portable/Lowering/UseTwiceRewriter.vb

+        End Function
+
+        Private Shared Function IsInvariantArray(type As TypeSymbol) As Boolean
+            Dim value = TryCast(type, ArrayTypeSymbol)?.ElementType?.IsNotInheritable


? [](start = 67, length = 1)

The second conditional access feels redundant. #Closed

AlekseyTs · 2018-01-09T19:26:03Z

src/Compilers/VisualBasic/Portable/Lowering/UseTwiceRewriter.vb

+
+        Private Shared Function IsInvariantArray(type As TypeSymbol) As Boolean
+            Dim value = TryCast(type, ArrayTypeSymbol)?.ElementType?.IsNotInheritable
+            Return value.HasValue AndAlso value.GetValueOrDefault()


value.HasValue AndAlso [](start = 19, length = 22)

The value.HasValue check is redundant. #Closed

agocke

LGTM

AlekseyTs · 2018-01-09T19:55:51Z

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

+        End Sub
+
+        <Fact>
+        Public Sub ArrayElementWithBlock_Invariant()


ArrayElementWithBlock_Invariant [](start = 19, length = 31)

I think that "With Block" tests would be more interesting if they were accessing the implicit receiver more than once. #Closed

AlekseyTs · 2018-01-09T20:01:25Z

Done with review pass (iteration 10) #Closed

AlekseyTs

LGTM (iteration 12)

cston · 2018-01-09T21:15:16Z

@dotnet-bot test windows_release_vs-integration_prtest

cston · 2018-01-09T21:15:39Z

@MeiChin-Tsai for approval.

MeiChin-Tsai · 2018-01-10T16:53:09Z

approved.

Avoid using a ref local for array element l-value

30b2023

Update test

823b04e

AlekseyTs reviewed Jan 6, 2018

View reviewed changes

VSadov reviewed Jan 6, 2018

View reviewed changes

cston added 2 commits January 8, 2018 08:59

Avoid ref local for array access only

f4cb9c2

PR feedback

ec674dc

cston added 2 commits January 8, 2018 13:31

Add test

b533a98

Misc.

0b89c5b

agocke reviewed Jan 8, 2018

View reviewed changes

VSadov reviewed Jan 8, 2018

View reviewed changes

Fix unit tests

178af07

VSadov reviewed Jan 8, 2018

View reviewed changes

VSadov approved these changes Jan 8, 2018

View reviewed changes

cston added 3 commits January 8, 2018 16:40

Add non-constant index tests

85118c2

Fix unit tests (again)

4c47f27

PR feedback

a8aa475

cston changed the base branch from master to dev15.6.x January 9, 2018 16:55

AlekseyTs reviewed Jan 9, 2018

View reviewed changes

agocke approved these changes Jan 9, 2018

View reviewed changes

AlekseyTs reviewed Jan 9, 2018

View reviewed changes

PR feedback

8d15538

Update With block tests

f657ba1

AlekseyTs approved these changes Jan 9, 2018

View reviewed changes

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

cston merged commit 2f6c1c5 into dotnet:dev15.6.x Jan 10, 2018

cston deleted the 547533 branch January 10, 2018 16:56

		@@ -1,5 +1,7 @@
		' Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

		#Const DONT_USE_BYREF_LOCALS_FOR_USE_TWICE = True

		@@ -208,7 +208,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic

		Debug.Assert(node.IsLValue)

		#If DONT_USE_BYREF_LOCALS_FOR_USE_TWICE Then

Avoid using a ref local for array element l-value #24070

Avoid using a ref local for array element l-value #24070

Conversation

cston commented Jan 5, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

How was the bug found?

cston commented Jan 5, 2018

AlekseyTs commented Jan 6, 2018 • edited Loading

AlekseyTs Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 6, 2018 • edited Loading

AlekseyTs commented Jan 6, 2018 • edited Loading

cston commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

cston commented Jan 9, 2018

cston commented Jan 9, 2018

AlekseyTs Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

AlekseyTs Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 9, 2018 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Jan 9, 2018

cston commented Jan 9, 2018

MeiChin-Tsai commented Jan 10, 2018

cston commented Jan 5, 2018 •

edited

Loading

AlekseyTs commented Jan 6, 2018 •

edited

Loading

AlekseyTs Jan 6, 2018 •

edited

Loading

VSadov Jan 6, 2018 •

edited

Loading

AlekseyTs commented Jan 6, 2018 •

edited

Loading

AlekseyTs commented Jan 6, 2018 •

edited

Loading

cston Jan 9, 2018 •

edited

Loading

AlekseyTs commented Jan 8, 2018 •

edited

Loading

AlekseyTs Jan 9, 2018 •

edited

Loading

AlekseyTs Jan 9, 2018 •

edited

Loading

AlekseyTs Jan 9, 2018 •

edited

Loading

AlekseyTs Jan 9, 2018 •

edited

Loading

AlekseyTs Jan 9, 2018 •

edited

Loading

AlekseyTs commented Jan 9, 2018 •

edited

Loading