From 0057a2f81e3a48e4b3e15bbb2f1b78d8ceb399e6 Mon Sep 17 00:00:00 2001 From: Joseph Tremoulet Date: Wed, 24 May 2017 12:23:52 -0400 Subject: [PATCH 1/2] Update full-struct references to promoted IBR args Update fgMorphImplicitByRefArgs to rewrite references to struct-promoted implicit-by-reference parameters as references to the new struct temps created in fgRetypeImplicitByRefArgs; fgMorphStructField isn't updating these because it's only interested in field references, and runs upstream of fgRetypeImplicitByRefArgs where the full struct temp is created, anyway. Invert the sense of lvPromoted for implicit byref args in the interim between fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs, since now fgMarkDemotedImplicitByRefArgs needs to update both and would look horribly backwards the other way around. Fixes #11814. --- src/jit/compiler.h | 3 +- src/jit/morph.cpp | 136 +++++++++++------- .../JitBlue/GitHub_11814/GitHub_11814.cs | 45 ++++++ .../JitBlue/GitHub_11814/GitHub_11814.csproj | 43 ++++++ 4 files changed, 174 insertions(+), 53 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.csproj 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 + + + + + + + + + + + From 5e61addb63e2c3714c0bea6217239915112f25bc Mon Sep 17 00:00:00 2001 From: Joseph Tremoulet Date: Wed, 24 May 2017 15:37:15 -0400 Subject: [PATCH 2/2] Improve comments around implicit byref rewrite A number of fields on the LclVarDsc get hijacked during the multi-stage rewrite of implicit-by-reference parameters; add comments at the fields' declarations as well as the hijacking uses. --- src/jit/compiler.h | 26 ++++++++++++++++++-------- src/jit/morph.cpp | 6 ++++-- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 9cd7b033f9da..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 diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 6617d7ac9085..3f32150d1725 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -17974,11 +17974,13 @@ void Compiler::fgMarkDemotedImplicitByRefArgs() { if (varDsc->lvPromoted) { - // The parameter is simply a pointer now, so clear 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 how to rewrite appearances of this arg. + // to tell fgMorphImplicitByRefArgs which local is the new promoted struct one. varDsc->lvFieldLclStart = 0; } else if (varDsc->lvFieldLclStart != 0)