diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 6f38e00160da..c8ead479b676 100644 --- a/src/jit/compiler.h +++ b/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; diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 086d469207ad..3f32150d1725 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,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; } 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 + + + + + + + + + + +