From eef7e7e9910a70ab4873fc3940b2da148f4f99e1 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Thu, 21 Feb 2019 18:14:48 -0800 Subject: [PATCH] Fix condition for calling EvalArgsToTemps `fgArgInfo::ArgsComplete()` checks for additional conditions requiring temps that are not checked for in the body of `fgMorphArgs()`. However, if there are no register args, it won't be called. Fix dotnet/coreclr#19256 Commit migrated from https://github.com/dotnet/coreclr/commit/30996b8a71d5734c75cb73a0973b9271f28d8424 --- src/coreclr/src/jit/compiler.h | 5 +++ src/coreclr/src/jit/importer.cpp | 9 ++--- src/coreclr/src/jit/morph.cpp | 31 ++++++++++++------ .../JitBlue/GitHub_19256/GitHub_19256.cs | 38 ++++++++++++++++++++++ .../JitBlue/GitHub_19256/GitHub_19256.csproj | 34 +++++++++++++++++++ 5 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.csproj diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index ab98059..e0c5e8d 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -1632,6 +1632,7 @@ class fgArgInfo bool hasStackArgs; // true if we have one or more stack arguments bool argsComplete; // marker for state bool argsSorted; // marker for state + bool needsTemps; // one or more arguments must be copied to a temp by EvalArgsToTemps fgArgTabEntry** argTable; // variable sized array of per argument descrption: (i.e. argTable[argTableSize]) private: @@ -1701,6 +1702,10 @@ public: { return hasRegArgs; } + bool NeedsTemps() + { + return needsTemps; + } bool HasStackArgs() { return hasStackArgs; diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 6f19213..4399381 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -1357,15 +1357,16 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr, assert(varTypeIsStruct(src->gtOp.gtOp2) || src->gtOp.gtOp2->gtType == TYP_BYREF); if (pAfterStmt) { + // Insert op1 after '*pAfterStmt' *pAfterStmt = fgInsertStmtAfter(block, *pAfterStmt, gtNewStmt(src->gtOp.gtOp1, ilOffset)); + // Evaluate the second thing using recursion. + return impAssignStructPtr(destAddr, src->gtOp.gtOp2, structHnd, curLevel, pAfterStmt, ilOffset, block); } else { - impAppendTree(src->gtOp.gtOp1, curLevel, ilOffset); // do the side effect + // We don't have an instruction to insert after, so use the entire comma expression as our rhs. + asgType = impNormStructType(structHnd); } - - // Evaluate the second thing using recursion. - return impAssignStructPtr(destAddr, src->gtOp.gtOp2, structHnd, curLevel, pAfterStmt, ilOffset, block); } else if (src->IsLocal()) { diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index ff9320ab..6551836 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -907,6 +907,7 @@ fgArgInfo::fgArgInfo(Compiler* comp, GenTreeCall* call, unsigned numArgs) hasStackArgs = false; argsComplete = false; argsSorted = false; + needsTemps = false; if (argTableSize == 0) { @@ -1466,6 +1467,7 @@ void fgArgInfo::ArgsComplete() ) { curArgTabEntry->needTmp = true; + needsTemps = true; } // For all previous arguments, unless they are a simple constant @@ -1479,6 +1481,7 @@ void fgArgInfo::ArgsComplete() if (prevArgTabEntry->node->gtOper != GT_CNS_INT) { prevArgTabEntry->needTmp = true; + needsTemps = true; } } } @@ -1524,11 +1527,13 @@ void fgArgInfo::ArgsComplete() if (argCount > 1) // If this is not the only argument { curArgTabEntry->needTmp = true; + needsTemps = true; } else if (varTypeIsFloating(argx->TypeGet()) && (argx->OperGet() == GT_CALL)) { // Spill all arguments that are floating point calls curArgTabEntry->needTmp = true; + needsTemps = true; } // All previous arguments may need to be evaluated into temps @@ -1543,6 +1548,7 @@ void fgArgInfo::ArgsComplete() if ((prevArgTabEntry->node->gtFlags & GTF_ALL_EFFECT) != 0) { prevArgTabEntry->needTmp = true; + needsTemps = true; } #if FEATURE_FIXED_OUT_ARGS // Or, if they are stored into the FIXED_OUT_ARG area @@ -1579,6 +1585,7 @@ void fgArgInfo::ArgsComplete() { // Spill multireg struct arguments that have Assignments or Calls embedded in them curArgTabEntry->needTmp = true; + needsTemps = true; } else { @@ -1589,6 +1596,7 @@ void fgArgInfo::ArgsComplete() { // Spill multireg struct arguments that are expensive to evaluate twice curArgTabEntry->needTmp = true; + needsTemps = true; } #if defined(FEATURE_SIMD) && defined(_TARGET_ARM64_) else if (isMultiRegArg && varTypeIsSIMD(argx->TypeGet())) @@ -1599,6 +1607,7 @@ void fgArgInfo::ArgsComplete() argx->AsObj()->gtOp1->gtOp.gtOp1->OperIsSimdOrHWintrinsic())) { curArgTabEntry->needTmp = true; + needsTemps = true; } } #endif @@ -1625,6 +1634,7 @@ void fgArgInfo::ArgsComplete() // For now we use a a GT_CPBLK to copy the exact size into a GT_LCL_VAR temp. // curArgTabEntry->needTmp = true; + needsTemps = true; } break; case 11: @@ -1641,6 +1651,7 @@ void fgArgInfo::ArgsComplete() // the argument. // curArgTabEntry->needTmp = true; + needsTemps = true; break; default: @@ -1704,6 +1715,7 @@ void fgArgInfo::ArgsComplete() if (argx->gtFlags & GTF_EXCEPT) { curArgTabEntry->needTmp = true; + needsTemps = true; continue; } #else @@ -1718,6 +1730,7 @@ void fgArgInfo::ArgsComplete() if (compiler->fgWalkTreePre(&argx, Compiler::fgChkLocAllocCB) == Compiler::WALK_ABORT) { curArgTabEntry->needTmp = true; + needsTemps = true; continue; } } @@ -1730,6 +1743,7 @@ void fgArgInfo::ArgsComplete() if (compiler->fgWalkTreePre(&argx, Compiler::fgChkQmarkCB) == Compiler::WALK_ABORT) { curArgTabEntry->needTmp = true; + needsTemps = true; continue; } } @@ -2187,7 +2201,7 @@ GenTree* Compiler::fgMakeTmpArgNode(fgArgTabEntry* curArgTabEntry) void fgArgInfo::EvalArgsToTemps() { - assert(argsSorted == true); + assert(argsSorted); unsigned regArgInx = 0; // Now go through the argument table and perform the necessary evaluation into temps @@ -3647,6 +3661,7 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) #ifdef DEBUG if (verbose) { + JITDUMP("ArgTable for %d.%s after fgInitArgInfo:\n", call->gtTreeID, GenTree::OpName(call->gtOper)); call->fgArgInfo->Dump(this); JITDUMP("\n"); } @@ -4270,17 +4285,12 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) // Union in the side effect flags from the call's operands call->gtFlags |= flagsSummary & GTF_ALL_EFFECT; - // If the register arguments have already been determined - // or we have no register arguments then we don't need to - // call SortArgs() and EvalArgsToTemps() + // If we are remorphing or don't have any register arguments or other arguments that need + // temps, then we don't need to call SortArgs() and EvalArgsToTemps(). // - // For UNIX_AMD64, the condition without hasStackArgCopy cannot catch - // all cases of fgMakeOutgoingStructArgCopy() being called. hasStackArgCopy - // is added to make sure to call EvalArgsToTemp. - if (!reMorphing && (call->fgArgInfo->HasRegArgs())) + if (!reMorphing && (call->fgArgInfo->HasRegArgs() || call->fgArgInfo->NeedsTemps())) { - // This is the first time that we morph this call AND it has register arguments. - // Follow into the code below and do the 'defer or eval to temp' analysis. + // Do the 'defer or eval to temp' analysis. call->fgArgInfo->SortArgs(); @@ -4301,6 +4311,7 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) #ifdef DEBUG if (verbose) { + JITDUMP("ArgTable for %d.%s after fgMorphArgs:\n", call->gtTreeID, GenTree::OpName(call->gtOper)); call->fgArgInfo->Dump(this); JITDUMP("\n"); } diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.cs new file mode 100644 index 0000000..261c07c --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.cs @@ -0,0 +1,38 @@ +// 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; +using System.Runtime.CompilerServices; + + +struct S2 +{ + public uint F0; + public ulong F1, F2; + public S2(uint f0) : this() { F0 = f0; } +} + +public class GitHub_19256 +{ + static S2 s_one = new S2(1); + static S2 s_two = new S2(2); + static uint sum = 0; + public static int Main() + { + M28(s_two, M28(s_one, s_one)); + return sum == 3 ? 100 : -1; + } + + static ref S2 M28(S2 arg0, S2 arg1) + { + sum += arg0.F0; + System.Console.WriteLine(arg0.F0); + arg0.F0 = 1234; // this is printed in next invocation + System.GC.KeepAlive(arg0); // ensure that assignment isn't removed + return ref s_one; + } +} + + diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.csproj new file mode 100644 index 0000000..c46f1c3 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19256/GitHub_19256.csproj @@ -0,0 +1,34 @@ + + + + + Debug + AnyCPU + 2.0 + {2649FAFE-07BF-4F93-8120-BA9A69285ABB} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + latest + None + True + + + + False + + + + + + + + + + + \ No newline at end of file -- 2.7.4