Update full-struct references to promoted IBR args

src/jit/compiler.h

@@ -255,7 +255,8 @@ class LclVarDsc
     unsigned char lvHasILStoreOp : 1;         // there is at least one STLOC or STARG on this local
     unsigned char lvHasMultipleILStoreOp : 1; // there is more than one STLOC on this local
 
-    unsigned char lvIsTemp : 1; // Short-lifetime compiler temp
+    unsigned char lvIsTemp : 1; // Short-lifetime compiler temp (if lvIsParam is false), or implicit byref parameter
+                                // (if lvIsParam is true)
 #if OPT_BOOL_OPS
     unsigned char lvIsBoolean : 1; // set if variable is boolean
 #endif
@@ -286,7 +287,9 @@ class LclVarDsc
                                // checks)
     unsigned char lvIsUnsafeBuffer : 1; // Does this contain an unsafe buffer requiring buffer overflow security checks?
     unsigned char lvPromoted : 1; // True when this local is a promoted struct, a normed struct, or a "split" long on a
-                                  // 32-bit target.
+                                  // 32-bit target.  For implicit byref parameters, this gets hijacked between
+                                  // fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs to indicate whether
+                                  // references to the arg are being rewritten as references to a promoted shadow local.
     unsigned char lvIsStructField : 1;          // Is this local var a field of a promoted struct local?
     unsigned char lvContainsFloatingFields : 1; // Does this struct contains floating point fields?
     unsigned char lvOverlappingFields : 1;      // True when we have a struct with possibly overlapping fields
@@ -332,7 +335,9 @@ class LclVarDsc
 
     union {
         unsigned lvFieldLclStart; // The index of the local var representing the first field in the promoted struct
-                                  // local.
+                                  // local.  For implicit byref parameters, this gets hijacked between
+                                  // fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs to point to the
+                                  // struct local created to model the parameter's struct promotion, if any.
         unsigned lvParentLcl; // The index of the local var representing the parent (i.e. the promoted struct local).
                               // Valid on promoted struct local fields.
     };
@@ -656,11 +661,16 @@ class LclVarDsc
 
     regMaskSmall lvPrefReg; // set of regs it prefers to live in
 
-    unsigned short lvVarIndex;  // variable tracking index
-    unsigned short lvRefCnt;    // unweighted (real) reference count
-    unsigned       lvRefCntWtd; // weighted reference count
-    int            lvStkOffs;   // stack offset of home
-    unsigned       lvExactSize; // (exact) size of the type in bytes
+    unsigned short lvVarIndex; // variable tracking index
+    unsigned short lvRefCnt;   // unweighted (real) reference count.  For implicit by reference
+                               // parameters, this gets hijacked from fgMarkImplicitByRefArgs
+                               // through fgMarkDemotedImplicitByRefArgs, to provide a static
+                               // appearance count (computed during address-exposed analysis)
+                               // that fgMakeOutgoingStructArgCopy consults during global morph
+                               // to determine if eliding its copy is legal.
+    unsigned lvRefCntWtd;      // weighted reference count
+    int      lvStkOffs;        // stack offset of home
+    unsigned lvExactSize;      // (exact) size of the type in bytes
 
     // Is this a promoted struct?
     // This method returns true only for structs (including SIMD structs), not for
@@ -4879,8 +4889,7 @@ class Compiler
     bool fgMorphImplicitByRefArgs(GenTreePtr tree);
     GenTreePtr fgMorphImplicitByRefArgs(GenTreePtr tree, bool isAddr);
 
-    // Clear up annotations for any struct promotion temps created for implicit byrefs that
-    // wound up unused (due e.g. to being address-exposed and not worth promoting).
+    // Clear up annotations for any struct promotion temps created for implicit byrefs.
     void fgMarkDemotedImplicitByRefArgs();
 
     static fgWalkPreFn  fgMarkAddrTakenLocalsPreCB;

src/jit/morph.cpp

@@ -17896,19 +17896,32 @@ void Compiler::fgRetypeImplicitByRefArgs()
                     fieldVarDsc->lvPrefReg = 0;
                 }
 
-                if (undoPromotion)
-                {
-                    // Hijack lvFieldLclStart to record the new temp number.
-                    // It will get fixed up in fgMarkDemotedImplicitByRefArgs.
-                    varDsc->lvFieldLclStart = newLclNum;
-                }
-                else
-                {
-                    // Unmark the parameter as promoted (it's a pointer now).
-                    varDsc->lvPromoted      = false;
-                    varDsc->lvFieldCnt      = 0;
-                    varDsc->lvFieldLclStart = 0;
-                }
+                // Hijack lvFieldLclStart to record the new temp number.
+                // It will get fixed up in fgMarkDemotedImplicitByRefArgs.
+                varDsc->lvFieldLclStart = newLclNum;
+                // Go ahead and clear lvFieldCnt -- either we're promoting
+                // a replacement temp or we're not promoting this arg, and
+                // in either case the parameter is now a pointer that doesn't
+                // have these fields.
+                varDsc->lvFieldCnt = 0;
+
+                // Hijack lvPromoted to communicate to fgMorphImplicitByRefArgs
+                // whether references to the struct should be rewritten as
+                // indirections off the pointer (not promoted) or references
+                // to the new struct local (promoted).
+                varDsc->lvPromoted = !undoPromotion;
+            }
+            else
+            {
+                // The "undo promotion" path above clears lvPromoted for args that struct
+                // promotion wanted to promote but that aren't considered profitable to
+                // rewrite.  It hijacks lvFieldLclStart to communicate to
+                // fgMarkDemotedImplicitByRefArgs that it needs to clean up annotations left
+                // on such args for fgMorphImplicitByRefArgs to consult in the interim.
+                // Here we have an arg that was simply never promoted, so make sure it doesn't
+                // have nonzero lvFieldLclStart, since that would confuse fgMorphImplicitByRefArgs
+                // and fgMarkDemotedImplicitByRefArgs.
+                assert(varDsc->lvFieldLclStart == 0);
             }
 
             // Since the parameter in this position is really a pointer, its type is TYP_BYREF.
@@ -17945,10 +17958,9 @@ void Compiler::fgRetypeImplicitByRefArgs()
 
 //------------------------------------------------------------------------
 // fgMarkDemotedImplicitByRefArgs: Clear annotations for any implicit byrefs that struct promotion
-//                                 asked to promote but for which fgRetypeImplicitByRefArgs decided
-//                                 to discard the promotion.  Appearances of these have now been
-//                                 rewritten (by fgMorphImplicitByRefArgs) using indirections from
-//                                 the pointer parameter.
+//                                 asked to promote.  Appearances of these have now been rewritten
+//                                 (by fgMorphImplicitByRefArgs) using indirections from the pointer
+//                                 parameter or references to the promotion temp, as appropriate.
 
 void Compiler::fgMarkDemotedImplicitByRefArgs()
 {
@@ -17958,45 +17970,60 @@ void Compiler::fgMarkDemotedImplicitByRefArgs()
     {
         LclVarDsc* varDsc = &lvaTable[lclNum];
 
-        if (lvaIsImplicitByRefLocal(lclNum) && varDsc->lvPromoted)
-        {
-            // We stashed the pointer to the real promotion temp in lvFieldLclStart
-            unsigned structLclNum = varDsc->lvFieldLclStart;
-
-            // Unmark the parameter as promoted.
-            varDsc->lvPromoted      = false;
-            varDsc->lvFieldCnt      = 0;
-            varDsc->lvFieldLclStart = 0;
-            // Clear its ref count; this was set during address-taken analysis so that
-            // call morphing could identify single-use implicit byrefs; we're done with
-            // that, and want it to be in its default state of zero when we go to set
-            // real ref counts for all variables.
-            varDsc->lvRefCnt = 0;
-
-            // The temp struct is now unused; set flags appropriately so that we
-            // won't allocate space for it on the stack.
-            LclVarDsc* structVarDsc     = &lvaTable[structLclNum];
-            structVarDsc->lvRefCnt      = 0;
-            structVarDsc->lvAddrExposed = false;
+        if (lvaIsImplicitByRefLocal(lclNum))
+        {
+            if (varDsc->lvPromoted)
+            {
+                // The parameter is simply a pointer now, so clear lvPromoted.  It was left set
+                // by fgRetypeImplicitByRefArgs to communicate to fgMorphImplicitByRefArgs that
+                // appearances of this arg needed to be rewritten to a new promoted struct local.
+                varDsc->lvPromoted = false;
+
+                // Clear the lvFieldLclStart value that was set by fgRetypeImplicitByRefArgs
+                // to tell fgMorphImplicitByRefArgs which local is the new promoted struct one.
+                varDsc->lvFieldLclStart = 0;
+            }
+            else if (varDsc->lvFieldLclStart != 0)
+            {
+                // We created new temps to represent a promoted struct corresponding to this
+                // parameter, but decided not to go through with the promotion and have
+                // rewritten all uses as indirections off the pointer parameter.
+                // We stashed the pointer to the new struct temp in lvFieldLclStart; make
+                // note of that and clear the annotation.
+                unsigned structLclNum   = varDsc->lvFieldLclStart;
+                varDsc->lvFieldLclStart = 0;
+
+                // Clear the arg's ref count; this was set during address-taken analysis so that
+                // call morphing could identify single-use implicit byrefs; we're done with
+                // that, and want it to be in its default state of zero when we go to set
+                // real ref counts for all variables.
+                varDsc->lvRefCnt = 0;
+
+                // The temp struct is now unused; set flags appropriately so that we
+                // won't allocate space for it on the stack.
+                LclVarDsc* structVarDsc     = &lvaTable[structLclNum];
+                structVarDsc->lvRefCnt      = 0;
+                structVarDsc->lvAddrExposed = false;
 #ifdef DEBUG
-            structVarDsc->lvUnusedStruct = true;
+                structVarDsc->lvUnusedStruct = true;
 #endif // DEBUG
 
-            unsigned fieldLclStart = structVarDsc->lvFieldLclStart;
-            unsigned fieldCount    = structVarDsc->lvFieldCnt;
-            unsigned fieldLclStop  = fieldLclStart + fieldCount;
+                unsigned fieldLclStart = structVarDsc->lvFieldLclStart;
+                unsigned fieldCount    = structVarDsc->lvFieldCnt;
+                unsigned fieldLclStop  = fieldLclStart + fieldCount;
 
-            for (unsigned fieldLclNum = fieldLclStart; fieldLclNum < fieldLclStop; ++fieldLclNum)
-            {
-                // Fix the pointer to the parent local.
-                LclVarDsc* fieldVarDsc = &lvaTable[fieldLclNum];
-                assert(fieldVarDsc->lvParentLcl == lclNum);
-                fieldVarDsc->lvParentLcl = structLclNum;
+                for (unsigned fieldLclNum = fieldLclStart; fieldLclNum < fieldLclStop; ++fieldLclNum)
+                {
+                    // Fix the pointer to the parent local.
+                    LclVarDsc* fieldVarDsc = &lvaTable[fieldLclNum];
+                    assert(fieldVarDsc->lvParentLcl == lclNum);
+                    fieldVarDsc->lvParentLcl = structLclNum;
 
-                // The field local is now unused; set flags appropriately so that
-                // we won't allocate stack space for it.
-                fieldVarDsc->lvRefCnt      = 0;
-                fieldVarDsc->lvAddrExposed = false;
+                    // The field local is now unused; set flags appropriately so that
+                    // we won't allocate stack space for it.
+                    fieldVarDsc->lvRefCnt      = 0;
+                    fieldVarDsc->lvAddrExposed = false;
+                }
             }
         }
     }
@@ -18074,8 +18101,17 @@ GenTreePtr Compiler::fgMorphImplicitByRefArgs(GenTreePtr tree, bool isAddr)
         if (!varTypeIsStruct(lclVarTree))
         {
             assert(lclVarTree->TypeGet() == TYP_BYREF);
+
             return nullptr;
         }
+        else if (lclVarDsc->lvPromoted)
+        {
+            // fgRetypeImplicitByRefArgs created a new promoted struct local to represent this
+            // arg.  Rewrite this to refer to the new local.
+            assert(lclVarDsc->lvFieldLclStart != 0);
+            lclVarTree->AsLclVarCommon()->SetLclNum(lclVarDsc->lvFieldLclStart);
+            return tree;
+        }
 
         fieldHnd = nullptr;
     }

tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.cs

@@ -0,0 +1,45 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// Repro case for a bug involving failure to rewrite all references
+// to a promoted implicit byref struct parameter.
+
+using System;
+using System.Runtime.CompilerServices;
+
+class MutateStructArg
+{
+    public struct P
+    {
+        public string S;
+        public int X;
+    }
+
+    public static int Main()
+    {
+        P l1 = new P();
+        l1.S = "Hello World";
+        l1.X = 42;
+        P l2 = foo(l1);
+        Console.WriteLine(l2.S); // Print modified value "Goodbye World"
+        if ((l2.S == "Goodbye World") && (l2.X == 100))
+        {
+            return 100;   // success
+        }
+        else
+        {
+            Console.WriteLine("**** Test FAILED ***");
+            return 1;  // failure
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static P foo(P a)
+    {
+        Console.WriteLine(a.S);   // Print the incoming value "Hello World"
+        a.S = "Goodbye World";    // Mutate the incoming value
+        a.X = 100;
+        return a;                 // Copy the modified value to the return value (bug was that this was returning original unmodified arg)
+    }
+}

tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.csproj

@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{7B521917-193E-48BB-86C6-FE013F3DFF35}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <AppDesignerFolder>Properties</AppDesignerFolder>
+    <FileAlignment>512</FileAlignment>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+
+    <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="GitHub_11814.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup>
+</Project>