Merge pull request #15516 from VSadov/fix15425

Optimizer should classify assignments to PseudoVariables as indirect assignments

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

@@ -969,21 +969,24 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
         private static bool IsIndirectAssignment(BoundAssignmentOperator node)
         {
             var lhs = node.Left;
+
+            Debug.Assert(node.RefKind == RefKind.None || (lhs as BoundLocal)?.LocalSymbol.RefKind == RefKind.Ref,
+                                "only ref locals can be a target of a ref assignment");
+            
             switch (lhs.Kind)
             {
                 case BoundKind.ThisReference:
-                    Debug.Assert(lhs.Type.IsValueType && node.RefKind == RefKind.None);
+                    Debug.Assert(lhs.Type.IsValueType, "'this' is assignable only in structs");
                     return true;
 
                 case BoundKind.Parameter:
                     if (((BoundParameter)lhs).ParameterSymbol.RefKind != RefKind.None)
                     {
                         bool isIndirect = node.RefKind == RefKind.None;
-                        Debug.Assert(isIndirect, "direct assignment to a ref/out parameter is highly suspicious");
                         return isIndirect;
                     }
 
-                    break;
+                    return false;
 
                 case BoundKind.Local:
                     if (((BoundLocal)lhs).LocalSymbol.RefKind != RefKind.None)
@@ -992,21 +995,39 @@ private static bool IsIndirectAssignment(BoundAssignmentOperator node)
                         return isIndirect;
                     }
 
-                    break;
+                    return false;
 
                 case BoundKind.Call:
                     Debug.Assert(((BoundCall)lhs).Method.RefKind == RefKind.Ref, "only ref returning methods are assignable");
-                    Debug.Assert(node.RefKind == RefKind.None, "methods cannot be ref-assignable");
                     return true;
 
                 case BoundKind.AssignmentOperator:
                     Debug.Assert(((BoundAssignmentOperator)lhs).RefKind == RefKind.Ref, "only ref assignments are assignable");
-                    Debug.Assert(node.RefKind == RefKind.None, "assignments cannot be ref-assignable");
                     return true;
-            }
 
-            Debug.Assert(node.RefKind == RefKind.None, "this is not something that can be assigned indirectly");
-            return false;
+                case BoundKind.Sequence:
+                    Debug.Assert(!IsIndirectAssignment(node.Update(((BoundSequence)node.Left).Value, node.Right, node.RefKind, node.Type)), 
+                        "indirect assignment to a sequence is unexpected");
+                    return false;
+
+                case BoundKind.RefValueOperator:
+                case BoundKind.PointerIndirectionOperator:
+                case BoundKind.PseudoVariable:
+                    return true;
+
+                case BoundKind.ModuleVersionId:
+                case BoundKind.InstrumentationPayloadRoot:
+                    // these are just stores into special static fields
+                    goto case BoundKind.FieldAccess;
+
+                case BoundKind.FieldAccess:
+                case BoundKind.ArrayAccess:
+                    // always symbolic stores
+                    return false;
+
+                default:
+                    throw ExceptionUtilities.UnexpectedValue(lhs.Kind);
+            }
         }
         private static bool IsIndirectOrInstanceFieldAssignment(BoundAssignmentOperator node)
         {
@@ -1958,13 +1979,12 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
 
             // indirect local store is not special. (operands still could be rewritten) 
             // NOTE: if Lhs is a stack local, it will be handled as a read and possibly duped.
-            var indirectStore = left.LocalSymbol.RefKind != RefKind.None && node.RefKind == RefKind.None;
-            if (indirectStore)
+            var isIndirectLocalStore = left.LocalSymbol.RefKind != RefKind.None && node.RefKind == RefKind.None;
+            if (isIndirectLocalStore)
             {
                 return base.VisitAssignmentOperator(node);
             }
 
-
             // ==  here we have a regular write to a stack local
             //
             // we do not need to visit lhs, because we do not read the local,

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

@@ -379,7 +379,7 @@ private BoundDynamicIndexerAccess TransformDynamicIndexerAccess(BoundDynamicInde
                 if (CanChangeValueBetweenReads(arguments[i]))
                 {
                     BoundAssignmentOperator assignmentToTemp;
-                    var temp = _factory.StoreToTemp(VisitExpression(arguments[i]), out assignmentToTemp, refKind: indexerAccess.ArgumentRefKindsOpt.RefKinds(i));
+                    var temp = _factory.StoreToTemp(VisitExpression(arguments[i]), out assignmentToTemp, indexerAccess.ArgumentRefKindsOpt.RefKinds(i) != RefKind.None ? RefKind.Ref : RefKind.None);
                     stores.Add(assignmentToTemp);
                     temps.Add(temp.LocalSymbol);
                     loweredArguments[i] = temp;

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

@@ -1245,6 +1245,11 @@ internal BoundExpression NullOrDefault(TypeSymbol typeSymbol)
 #endif
             )
         {
+            if (refKind == RefKind.Out)
+            {
+                refKind = RefKind.Ref;
+            }
+
             MethodSymbol containingMethod = this.CurrentMethod;
             var syntax = argument.Syntax;
             var type = argument.Type;

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedLocal.cs

@@ -37,6 +37,7 @@ internal sealed class SynthesizedLocal : LocalSymbol
         {
             Debug.Assert(type.SpecialType != SpecialType.System_Void);
             Debug.Assert(!kind.IsLongLived() || syntaxOpt != null);
+            Debug.Assert(refKind != RefKind.Out);
 
             _containingMethodOpt = containingMethodOpt;
             _type = type;

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

@@ -140,18 +140,49 @@ class Program
     {
         return ref P;
     }
+
+    public static void Main()
+    {
+        var local = 42;   // must be real local
+        P = local;
+        P = local;  // assign again, should not use stack local
+        System.Console.WriteLine(P);
+    }
 }
 ";
 
-            CompileAndVerifyRef(text).VerifyIL("Program.M()", @"
+            var v = CompileAndVerifyRef(text, expectedOutput: "42");
+
+            v.VerifyIL("Program.M()", @"
 {
   // Code size        6 (0x6)
   .maxstack  1
   IL_0000:  call       ""ref int Program.P.get""
   IL_0005:  ret
 }");
+
+            v.VerifyIL("Program.Main()", @"
+{
+  // Code size       29 (0x1d)
+  .maxstack  2
+  .locals init (int V_0) //local
+  IL_0000:  ldc.i4.s   42
+  IL_0002:  stloc.0
+  IL_0003:  call       ""ref int Program.P.get""
+  IL_0008:  ldloc.0
+  IL_0009:  stind.i4
+  IL_000a:  call       ""ref int Program.P.get""
+  IL_000f:  ldloc.0
+  IL_0010:  stind.i4
+  IL_0011:  call       ""ref int Program.P.get""
+  IL_0016:  ldind.i4
+  IL_0017:  call       ""void System.Console.WriteLine(int)""
+  IL_001c:  ret
+}");
+
         }
 
+
         [Fact]
         public void RefReturnClassInstanceProperty()
         {

src/Compilers/CSharp/Test/Semantic/Semantics/ArglistTests.cs

@@ -794,6 +794,83 @@ static void Set(TypedReference tr, dynamic i)
             CreateCompilationWithMscorlibAndSystemCore(text).VerifyDiagnostics();
         }
 
+        [ClrOnlyFact(ClrOnlyReason.Ilasm)]
+        public void RefValueTest04_optimizer()
+        {
+            var text = @"
+using System;
+public struct C
+{
+    static void Main()
+    {
+        int k = 42;
+
+        int i = 1;
+        TypedReference tr1 = __makeref(i);
+
+        __refvalue(tr1, int) = k;
+        __refvalue(tr1, int) = k;
+
+        int j = 1;
+        TypedReference tr2 = __makeref(j);
+
+        int l = 42;
+
+        __refvalue(tr1, int) = l;
+        __refvalue(tr2, int) = l;
+
+        Console.Write(i);
+        Console.Write(j);
+    }
+}";
+
+            var verifier = CompileAndVerify(source: text, expectedOutput: "4242");
+            verifier.VerifyIL("C.Main", @"
+{
+  // Code size       72 (0x48)
+  .maxstack  3
+  .locals init (int V_0, //k
+                int V_1, //i
+                System.TypedReference V_2, //tr1
+                int V_3, //j
+                int V_4) //l
+  IL_0000:  ldc.i4.s   42
+  IL_0002:  stloc.0
+  IL_0003:  ldc.i4.1
+  IL_0004:  stloc.1
+  IL_0005:  ldloca.s   V_1
+  IL_0007:  mkrefany   ""int""
+  IL_000c:  stloc.2
+  IL_000d:  ldloc.2
+  IL_000e:  refanyval  ""int""
+  IL_0013:  ldloc.0
+  IL_0014:  stind.i4
+  IL_0015:  ldloc.2
+  IL_0016:  refanyval  ""int""
+  IL_001b:  ldloc.0
+  IL_001c:  stind.i4
+  IL_001d:  ldc.i4.1
+  IL_001e:  stloc.3
+  IL_001f:  ldloca.s   V_3
+  IL_0021:  mkrefany   ""int""
+  IL_0026:  ldc.i4.s   42
+  IL_0028:  stloc.s    V_4
+  IL_002a:  ldloc.2
+  IL_002b:  refanyval  ""int""
+  IL_0030:  ldloc.s    V_4
+  IL_0032:  stind.i4
+  IL_0033:  refanyval  ""int""
+  IL_0038:  ldloc.s    V_4
+  IL_003a:  stind.i4
+  IL_003b:  ldloc.1
+  IL_003c:  call       ""void System.Console.Write(int)""
+  IL_0041:  ldloc.3
+  IL_0042:  call       ""void System.Console.Write(int)""
+  IL_0047:  ret
+}
+");
+        }
+
         [Fact]
         public void TestBug13263()
         {

src/Compilers/VisualBasic/Portable/CodeGen/Optimizer/StackScheduler.Analyzer.vb

@@ -613,26 +613,45 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeGen
             End Function
 
             ''' <summary>
-            ''' indirect assignment is assignment to a value referenced indirectly
-            ''' it may only happen if lhs is a reference (must be a parameter or a local)
-            '''       1) lhs is a reference (must be a parameter or a local)
-            '''       2) it is not a ref/out assignment where the reference itself would be assigned
+            ''' VB uses a special node to assign references.
+            ''' BoundAssignment is used only to assign values.
+            ''' therefore an indirect assignment may only happen if lhs is a reference
             ''' </summary>
-            Friend Shared Function IsIndirectAssignment(node As BoundAssignmentOperator) As Boolean
-                If Not IsByRefLocalOrParameter(node.Left) Then
-                    Return False
-                End If
-                Return Not IsByRefLocalOrParameter(node.Right)
+            Private Shared Function IsIndirectAssignment(node As BoundAssignmentOperator) As Boolean
+                Return IsByRefVariable(node.Left)
             End Function
 
-            Private Shared Function IsByRefLocalOrParameter(node As BoundExpression) As Boolean
+            Private Shared Function IsByRefVariable(node As BoundExpression) As Boolean
                 Select Case node.Kind
                     Case BoundKind.Parameter
                         Return DirectCast(node, BoundParameter).ParameterSymbol.IsByRef
                     Case BoundKind.Local
                         Return DirectCast(node, BoundLocal).LocalSymbol.IsByRef
-                    Case Else
+                    Case BoundKind.Call
+                        Return DirectCast(node, BoundCall).Method.ReturnsByRef
+                    Case BoundKind.Sequence
+                        Debug.Assert(Not IsByRefVariable(DirectCast(node, BoundSequence).ValueOpt))
+                        Return False
+
+                    Case BoundKind.PseudoVariable
+                        Return True
+                    Case BoundKind.ReferenceAssignment
+                        Return True
+                    Case BoundKind.ValueTypeMeReference
+                        Return True
+
+                    Case BoundKind.ModuleVersionId,
+                        BoundKind.InstrumentationPayloadRoot
+                        ' same as static fields
+                        Return False
+
+                    Case BoundKind.FieldAccess,
+                         BoundKind.ArrayAccess
+                        ' fields are never byref
                         Return False
+
+                    Case Else
+                        Throw ExceptionUtilities.UnexpectedValue(node.Kind)
                 End Select
             End Function
 

src/Compilers/VisualBasic/Portable/CodeGen/Optimizer/StackScheduler.Rewriter.vb

@@ -181,7 +181,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeGen
 
                 ' indirect local store is not special. (operands still could be rewritten) 
                 ' NOTE: if Lhs is a stack local, it will be handled as a read and possibly duped.
-                If Analyzer.IsIndirectAssignment(node) Then
+                Dim isIndirectLocalStore = left.LocalSymbol.IsByRef
+                If isIndirectLocalStore Then
                     Return VisitAssignmentOperatorDefault(node)
                 End If
 

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

@@ -256,34 +256,40 @@ End Module",
                 Nothing,
 "Module M
     Sub Main()
-        C(Of Double).P = 1.5
+        Dim d = 1.5  ' must not be stack local
+        C(Of Double).P = d
+        C(Of Double).P = d  ' assign second time, should not be on stack
         C(Of Double).P += 2.0
         System.Console.Write(C(Of Double).P)
     End Sub
 End Module",
                 referencedCompilations:={comp1},
-                compilationOptions:=TestOptions.DebugExe)
+                compilationOptions:=TestOptions.ReleaseExe)
             Dim verifier = CompileAndVerify(comp2, expectedOutput:="3.5")
             verifier.VerifyIL("M.Main",
             <![CDATA[
 {
-  // Code size       51 (0x33)
+  // Code size       58 (0x3a)
   .maxstack  3
-  IL_0000:  nop
-  IL_0001:  call       "ByRef Function C(Of Double).get_P() As Double"
-  IL_0006:  ldc.r8     1.5
-  IL_000f:  stind.r8
-  IL_0010:  call       "ByRef Function C(Of Double).get_P() As Double"
-  IL_0015:  call       "ByRef Function C(Of Double).get_P() As Double"
-  IL_001a:  ldind.r8
-  IL_001b:  ldc.r8     2
-  IL_0024:  add
-  IL_0025:  stind.r8
-  IL_0026:  call       "ByRef Function C(Of Double).get_P() As Double"
-  IL_002b:  ldind.r8
-  IL_002c:  call       "Sub System.Console.Write(Double)"
-  IL_0031:  nop
-  IL_0032:  ret
+  .locals init (Double V_0) //d
+  IL_0000:  ldc.r8     1.5
+  IL_0009:  stloc.0
+  IL_000a:  call       "ByRef Function C(Of Double).get_P() As Double"
+  IL_000f:  ldloc.0
+  IL_0010:  stind.r8
+  IL_0011:  call       "ByRef Function C(Of Double).get_P() As Double"
+  IL_0016:  ldloc.0
+  IL_0017:  stind.r8
+  IL_0018:  call       "ByRef Function C(Of Double).get_P() As Double"
+  IL_001d:  call       "ByRef Function C(Of Double).get_P() As Double"
+  IL_0022:  ldind.r8
+  IL_0023:  ldc.r8     2
+  IL_002c:  add
+  IL_002d:  stind.r8
+  IL_002e:  call       "ByRef Function C(Of Double).get_P() As Double"
+  IL_0033:  ldind.r8
+  IL_0034:  call       "Sub System.Console.Write(Double)"
+  IL_0039:  ret
 }
 ]]>)
             verifier.VerifyDiagnostics()