From 0057a2f81e3a48e4b3e15bbb2f1b78d8ceb399e6 Mon Sep 17 00:00:00 2001 From: Joseph Tremoulet Date: Wed, 24 May 2017 12:23:52 -0400 Subject: [PATCH] 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 + + + + + + + + + + +