From: Brian Sullivan Date: Thu, 1 Nov 2018 00:11:38 +0000 (-0700) Subject: Fix constant propagation with nested structs X-Git-Tag: submit/tizen/20210909.063632~11030^2~3513^2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=aced63a262dca41d3e2d7e2ac102c773377f45e7;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fix constant propagation with nested structs Fixes dotnet/coreclr#18672 Test case is here: JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs Commit migrated from https://github.com/dotnet/coreclr/commit/adbc0f5f613eb72131d3224c24ef79ca589b1d06 --- diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp index ddc1a0e..b090b80 100644 --- a/src/coreclr/src/jit/valuenum.cpp +++ b/src/coreclr/src/jit/valuenum.cpp @@ -2235,7 +2235,6 @@ TailCall: } else { - // Give up if we've run out of budget. if (--(*pBudget) <= 0) { @@ -6082,8 +6081,8 @@ ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind memoryKind, assert(nonLoopPred != nullptr); // What is its memory post-state? ValueNum newMemoryVN = GetMemoryPerSsaData(nonLoopPred->bbMemorySsaNumOut[memoryKind])->m_vnPair.GetLiberal(); - assert(newMemoryVN != - ValueNumStore::NoVN); // We must have processed the single non-loop pred before reaching the loop entry. + assert(newMemoryVN != ValueNumStore::NoVN); // We must have processed the single non-loop pred before reaching the + // loop entry. #ifdef DEBUG if (verbose) @@ -6352,10 +6351,6 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) { GenTree* lhs = tree->gtGetOp1(); GenTree* rhs = tree->gtGetOp2(); -#ifdef DEBUG - // Sometimes we query the memory ssa map in an assertion, and need a dummy location for the ignored result. - unsigned memorySsaNum; -#endif if (tree->OperIsInitBlkOp()) { @@ -6366,7 +6361,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) { assert(lclVarTree->gtFlags & GTF_VAR_DEF); // Should not have been recorded as updating the GC heap. - assert(!GetMemorySsaMap(GcHeap)->Lookup(tree, &memorySsaNum)); + assert(!GetMemorySsaMap(GcHeap)->Lookup(tree)); unsigned lclNum = lclVarTree->GetLclNum(); @@ -6375,7 +6370,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) if (lvaInSsa(lclNum)) { // Should not have been recorded as updating ByrefExposed. - assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree, &memorySsaNum)); + assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree)); unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree); @@ -6435,7 +6430,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) if (tree->DefinesLocal(this, &lclVarTree, &isEntire)) { // Should not have been recorded as updating the GC heap. - assert(!GetMemorySsaMap(GcHeap)->Lookup(tree, &memorySsaNum)); + assert(!GetMemorySsaMap(GcHeap)->Lookup(tree)); unsigned lhsLclNum = lclVarTree->GetLclNum(); FieldSeqNode* lhsFldSeq = nullptr; @@ -6443,12 +6438,11 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) if (lvaInSsa(lhsLclNum)) { // Should not have been recorded as updating ByrefExposed. - assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree, &memorySsaNum)); + assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree)); unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree); - if (lhs->IsLocalExpr(this, &lclVarTree, &lhsFldSeq) || - (lhs->OperIsBlk() && (lhs->AsBlk()->gtBlkSize == lvaLclSize(lhsLclNum)))) + if (lhs->IsLocalExpr(this, &lclVarTree, &lhsFldSeq)) { noway_assert(lclVarTree->gtLclNum == lhsLclNum); } @@ -6599,12 +6593,15 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) } else if (lhsFldSeq != nullptr && isEntire) { - // This can occur in for structs with one field, itself of a struct type. - // We won't promote these. - // TODO-Cleanup: decide what exactly to do about this. - // Always treat them as maps, making them use/def, or reconstitute the - // map view here? - isNewUniq = true; + // This can occur for structs with one field, itself of a struct type. + // We are assigning the one field and it is also the entire enclosing struct. + // + // Use an unique value number for the old map, as this is an an entire assignment + // and we won't have any other values in the map + ValueNumPair oldMap; + oldMap.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet())); + rhsVNPair = vnStore->VNPairApplySelectorsAssign(oldMap, lhsFldSeq, rhsVNPair, lclVarTree->TypeGet(), + compCurBB); } else if (!isNewUniq) { @@ -8479,8 +8476,8 @@ void Compiler::fgValueNumberHelperCallFunc(GenTreeCall* call, VNFunc vnf, ValueN // Add the accumulated exceptions. call->gtVNPair = vnStore->VNPWithExc(call->gtVNPair, vnpExc); } - assert(args == nullptr || - generateUniqueVN); // All arguments should be processed or we generate unique VN and do not care. + assert(args == nullptr || generateUniqueVN); // All arguments should be processed or we generate unique VN and do + // not care. } void Compiler::fgValueNumberCall(GenTreeCall* call) diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs new file mode 100644 index 0000000..51695fd --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs @@ -0,0 +1,69 @@ +// 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. + +using System.Runtime.CompilerServices; + +struct S4 +{ + int FI1; +} + +struct S24 +{ + public int FI2; + public ulong FU3; + public S4 FS4; + public S24(int i, S4 s): this() + { + FI2 = i; + FS4 = s; + } +} + +// A 24-byte struct that simply wraps S24 +struct S24W +{ + public S24 FS24; + public S24W(S24 s): this() + { + FS24 = s; + } +} + +public class Program +{ + public static int Main() + { + S4 s4 = new S4(); + S24 s24 = new S24(1, s4); + S24W s24W = new S24W(s24); + S24 s24X = s24; + + int fI2 = s24.FI2; + int fI2W = s24W.FS24.FI2; + int fI2X = s24X.FI2; + + M(); + + if ((fI2 == 1) && (fI2W == 1) && (fI2X == 1)) + { + System.Console.WriteLine("Passed"); + return 100; + } + else + { + // Before the fix we would fail with: + // + System.Console.WriteLine(fI2); // 1 in debug, 1 in release OK + System.Console.WriteLine(fI2W); // 1 in debug, 0 in release BAD + System.Console.WriteLine(fI2X); // 1 in debug, 1 in release OK + + System.Console.WriteLine("Failed"); + return 101; + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void M(){} +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.csproj new file mode 100644 index 0000000..685e3e8 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.csproj @@ -0,0 +1,39 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + 1 + + + + + + + False + + + + + True + + + + + + + + + + \ No newline at end of file