diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 6f38e00160da..9cd7b033f9da 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -4879,8 +4879,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; diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 086d469207ad..6617d7ac9085 100644 --- a/src/jit/morph.cpp +++ b/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,58 @@ 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. + varDsc->lvPromoted = false; + + // Clear the lvFieldLclStart value that was set by fgRetypeImplicitByRefArgs + // to tell fgMorphImplicitByRefArgs how to rewrite appearances of this arg. + 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 +18099,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; } diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.cs b/tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.cs new file mode 100644 index 000000000000..16ca8c2f27eb --- /dev/null +++ b/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) + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.csproj new file mode 100644 index 000000000000..2db21106fd45 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.csproj @@ -0,0 +1,43 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {7B521917-193E-48BB-86C6-FE013F3DFF35} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + + 7a9bfb7d + + + + + + + + + False + + + + + True + True + + + + + + + + + + +