From 2fd2f37f8f9fa8ad4a066b27495a3af61e11674d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 1 Jun 2022 19:48:24 -0700 Subject: [PATCH 1/3] JIT: fix invariant analysis for cloning Fix some cases where the JIT was not sufficiently careful in verifing that operands in a loop were suitably invariant. Closes #61040. --- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/optimizer.cpp | 236 ++++++++++++++---- src/tests/JIT/opt/Cloning/Runtime_61040_1.cs | 53 ++++ .../JIT/opt/Cloning/Runtime_61040_1.csproj | 9 + src/tests/JIT/opt/Cloning/Runtime_61040_2.cs | 49 ++++ .../JIT/opt/Cloning/Runtime_61040_2.csproj | 9 + src/tests/JIT/opt/Cloning/Runtime_61040_3.cs | 45 ++++ .../JIT/opt/Cloning/Runtime_61040_3.csproj | 9 + src/tests/JIT/opt/Cloning/Runtime_61040_4.cs | 49 ++++ .../JIT/opt/Cloning/Runtime_61040_4.csproj | 9 + 10 files changed, 417 insertions(+), 55 deletions(-) create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_1.cs create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_1.csproj create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_2.cs create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_2.csproj create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_3.cs create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_3.csproj create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_4.cs create mode 100644 src/tests/JIT/opt/Cloning/Runtime_61040_4.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 34bb62a0daafe5..30d807a6c37361 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6306,7 +6306,7 @@ class Compiler bool optIsLoopTestEvalIntoTemp(Statement* testStmt, Statement** newTestStmt); unsigned optIsLoopIncrTree(GenTree* incr); - bool optCheckIterInLoopTest(unsigned loopInd, GenTree* test, BasicBlock* from, BasicBlock* to, unsigned iterVar); + bool optCheckIterInLoopTest(unsigned loopInd, GenTree* test, unsigned iterVar); bool optComputeIterInfo(GenTree* incr, BasicBlock* from, BasicBlock* to, unsigned* pIterVar); bool optPopulateInitInfo(unsigned loopInd, BasicBlock* initBlock, GenTree* init, unsigned iterVar); bool optExtractInitTestIncr( @@ -6400,7 +6400,7 @@ class Compiler bool optIsVarAssgLoop(unsigned lnum, unsigned var); - int optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKinds inds = VR_NONE); + bool optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKinds inds = VR_NONE); bool optNarrowTree(GenTree* tree, var_types srct, var_types dstt, ValueNumPair vnpNarrow, bool doit); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index e58f1c908bcd96..1a748c2d4984c7 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -839,10 +839,9 @@ bool Compiler::optPopulateInitInfo(unsigned loopInd, BasicBlock* initBlock, GenT // optCheckIterInLoopTest: Check if iter var is used in loop test. // // Arguments: +// loopInd loopIndex // test "jtrue" tree or an asg of the loop iter termination condition -// from/to blocks (beg, end) which are part of the loop. // iterVar loop iteration variable. -// loopInd loop index. // // Operation: // The test tree is parsed to check if "iterVar" matches the lhs of the condition @@ -853,8 +852,7 @@ bool Compiler::optPopulateInitInfo(unsigned loopInd, BasicBlock* initBlock, GenT // "false" if the loop table could not be populated with the loop test info or // if the test condition doesn't involve iterVar. // -bool Compiler::optCheckIterInLoopTest( - unsigned loopInd, GenTree* test, BasicBlock* from, BasicBlock* to, unsigned iterVar) +bool Compiler::optCheckIterInLoopTest(unsigned loopInd, GenTree* test, unsigned iterVar) { // Obtain the relop from the "test" tree. GenTree* relop; @@ -909,20 +907,46 @@ bool Compiler::optCheckIterInLoopTest( optLoopTable[loopInd].lpFlags |= LPFLG_SIMD_LIMIT; } } - else if (limitOp->gtOper == GT_LCL_VAR && - !optIsVarAssigned(from, to, nullptr, limitOp->AsLclVarCommon()->GetLclNum())) + else if (limitOp->gtOper == GT_LCL_VAR) { - optLoopTable[loopInd].lpFlags |= LPFLG_VAR_LIMIT; + // See if limit var is a loop invariant + // + if (!optIsVarAssgLoop(loopInd, limitOp->AsLclVarCommon()->GetLclNum())) + { + optLoopTable[loopInd].lpFlags |= LPFLG_VAR_LIMIT; + } + else + { + JITDUMP("Limit var %V02u modifiable in " FMT_LP "\n", limitOp->AsLclVarCommon()->GetLclNum(), loopInd); + } } else if (limitOp->gtOper == GT_ARR_LENGTH) { - optLoopTable[loopInd].lpFlags |= LPFLG_ARRLEN_LIMIT; + // See if limit array is a loop invariant + // + GenTree* const array = limitOp->AsArrLen()->ArrRef(); + + if (array->OperIs(GT_LCL_VAR)) + { + if (!optIsVarAssgLoop(loopInd, array->AsLclVarCommon()->GetLclNum())) + { + optLoopTable[loopInd].lpFlags |= LPFLG_ARRLEN_LIMIT; + } + } } else { + // Not clear why we return false here. We know the loop exit test + // is comparing iterVar to something; we just don't know what. + // + // No real difference between this case and the cases above where + // we don't set some kind of limit flag. + // return false; } + // Save the type of the comparison between the iterator and the limit. + // optLoopTable[loopInd].lpTestTree = relop; return true; } @@ -977,7 +1001,7 @@ unsigned Compiler::optIsLoopIncrTree(GenTree* incr) // optComputeIterInfo: Check tree is loop increment of a lcl that is loop-invariant. // // Arguments: -// from, to - are blocks (beg, end) which are part of the loop. +// lnum - loop in question // incr - tree that increments the loop iterator. v+=1 or v=v+1. // pIterVar - see return value. // @@ -987,22 +1011,48 @@ unsigned Compiler::optIsLoopIncrTree(GenTree* incr) // // Operation: // Check if the "incr" tree is a "v=v+1 or v+=1" type tree and make sure it is not -// assigned in the loop. +// otherwise modified in the loop. // bool Compiler::optComputeIterInfo(GenTree* incr, BasicBlock* from, BasicBlock* to, unsigned* pIterVar) { + const unsigned iterVar = optIsLoopIncrTree(incr); - unsigned iterVar = optIsLoopIncrTree(incr); if (iterVar == BAD_VAR_NUM) { return false; } + + // Note we can't use optIsVarAssgLoop here, as iterVar is indeed + // assigned within the loop. + // + // Bail on promoted case, otherwise we'd have to search the loop + // for both iterVar and its parent. + // + // Bail on the potentially aliased case. + // + LclVarDsc* const iterVarDsc = lvaGetDesc(iterVar); + + if (iterVarDsc->lvIsStructField) + { + JITDUMP("iterVar V%02u is a promoted field\n", iterVar); + return false; + } + + if (iterVarDsc->IsAddressExposed()) + { + JITDUMP("iterVar V%02u is address exposed\n", iterVar); + return false; + } + if (optIsVarAssigned(from, to, incr, iterVar)) { - JITDUMP("iterVar is assigned in loop\n"); + JITDUMP("iterVar V%02u is assigned in loop\n", iterVar); return false; } + JITDUMP("iterVar V%02u is invariant in loop (with the exception of the update in [%06u])\n", iterVar, + dspTreeID(incr)); + *pIterVar = iterVar; return true; } @@ -1282,6 +1332,9 @@ bool Compiler::optRecordLoop( } #endif // DEBUG + bool loopInsertedAtEnd = (loopInd == optLoopCount); + optLoopCount++; + optLoopTable[loopInd].lpHead = head; optLoopTable[loopInd].lpTop = top; optLoopTable[loopInd].lpBottom = bottom; @@ -1338,9 +1391,10 @@ bool Compiler::optRecordLoop( optPopulateInitInfo(loopInd, head, init, iterVar); // Check that the iterator is used in the loop condition. - if (!optCheckIterInLoopTest(loopInd, test, head->bbNext, bottom, iterVar)) + if (!optCheckIterInLoopTest(loopInd, test, iterVar)) { - JITDUMP(FMT_LP ": iterator not used in loop condition; not LPFLG_ITER loop\n", loopInd); + JITDUMP(FMT_LP ": iterator V%02u fails analysis of loop condition [%06u]; not LPFLG_ITER loop\n", loopInd, + iterVar, dspTreeID(test)); goto DONE_LOOP; } @@ -1365,9 +1419,6 @@ bool Compiler::optRecordLoop( DONE_LOOP: - bool loopInsertedAtEnd = (loopInd == optLoopCount); - optLoopCount++; - #ifdef DEBUG if (verbose) { @@ -5614,12 +5665,16 @@ bool Compiler::optNarrowTree(GenTree* tree, var_types srct, var_types dstt, Valu return false; } -/***************************************************************************** - * - * The following logic figures out whether the given variable is assigned - * somewhere in a list of basic blocks (or in an entire loop). - */ - +//------------------------------------------------------------------------ +// optIsVarAssgCB: callback to record local modification info +// +// Arguments: +// pTree - pointer to tree to scan +// data - ambient walk data +// +// Returns: +// standard walk result +// Compiler::fgWalkResult Compiler::optIsVarAssgCB(GenTree** pTree, fgWalkData* data) { GenTree* tree = *pTree; @@ -5671,20 +5726,20 @@ Compiler::fgWalkResult Compiler::optIsVarAssgCB(GenTree** pTree, fgWalkData* dat } else if (destOper == GT_LCL_FLD) { - /* We can't track every field of every var. Moreover, indirections - may access different parts of the var as different (but - overlapping) fields. So just treat them as indirect accesses */ - + // We can't track every field of every var. Moreover, indirections + // may access different parts of the var as different (but + // overlapping) fields. So just treat them as indirect accesses + // // unsigned lclNum = dest->AsLclFld()->GetLclNum(); // noway_assert(lvaTable[lclNum].lvAddrTaken); - + // varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL; desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs); } else if (destOper == GT_IND) { - /* Set the proper indirection bits */ - + // Set the proper indirection bits + // varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL; desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs); } @@ -5693,8 +5748,23 @@ Compiler::fgWalkResult Compiler::optIsVarAssgCB(GenTree** pTree, fgWalkData* dat return WALK_CONTINUE; } -/*****************************************************************************/ - +//------------------------------------------------------------------------ +// optIsVarAssigned: see if a local is assigned in a range of blocks +// +// Arguments: +// beg - first block in range +// end - last block in range +// skip - tree to ignore (nullptr if none) +// var - local to check +// +// Returns: +// true if local is directly modified +// +// Notes: +// Does a full walk of all blocks/statements/trees, so potentially expensive. +// +// Does not do proper checks for struct fields or exposed locals. +// bool Compiler::optIsVarAssigned(BasicBlock* beg, BasicBlock* end, GenTree* skip, unsigned var) { bool result; @@ -5736,38 +5806,97 @@ bool Compiler::optIsVarAssigned(BasicBlock* beg, BasicBlock* end, GenTree* skip, return result; } -/***************************************************************************** - * Is "var" assigned in the loop "lnum" ? - */ - +//------------------------------------------------------------------------ +// optIsVarAssgLoop: see if a local is assigned in a loop +// +// Arguments: +// lnum - loop number +// var - var to check +// +// Returns: +// true if var can possibly be modified in the loop specified by lnum +// false if var is a loop invariant +// bool Compiler::optIsVarAssgLoop(unsigned lnum, unsigned var) { assert(lnum < optLoopCount); + + LclVarDsc* const varDsc = lvaGetDesc(var); + if (varDsc->IsAddressExposed()) + { + // Assume the worst + // + return true; + } + if (var < lclMAX_ALLSET_TRACKED) { ALLVARSET_TP vs(AllVarSetOps::MakeSingleton(this, var)); + + // If local is a promoted field, also check for modifications to parent. + // + if (varDsc->lvIsStructField) + { + unsigned const parentVar = varDsc->lvParentLcl; + assert(!lvaGetDesc(parentVar)->IsAddressExposed()); + assert(lvaGetDesc(parentVar)->lvPromoted); + + if (parentVar < lclMAX_ALLSET_TRACKED) + { + JITDUMP("optIsVarAssgLoop: V%02u promoted, also checking V%02u\n", var, parentVar); + AllVarSetOps::AddElemD(this, vs, parentVar); + } + else + { + // Parent var index is too large, assume the worst. + // + return false; + } + } + return optIsSetAssgLoop(lnum, vs) != 0; } else { + if (varDsc->lvIsStructField) + { + return false; + } + return optIsVarAssigned(optLoopTable[lnum].lpHead->bbNext, optLoopTable[lnum].lpBottom, nullptr, var); } } -/*****************************************************************************/ -int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKinds inds) +//------------------------------------------------------------------------ +// optIsSetAssgLoop: see if a set of locals is assigned in a loop +// +// Arguments: +// lnum - loop number +// vars - var set to check +// inds - also consider impact of indirect stores and calls +// +// Returns: +// true if any of vars are possibly modified in any of the blocks of the +// loop specified by lnum, or if the loop contains any of the specified +// aliasing operations. +// +// Notes: +// Uses a cache to avoid repeatedly scanning the loop blocks. However this +// cache never invalidates and so this method must be used with care. +// +bool Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKinds inds) { noway_assert(lnum < optLoopCount); LoopDsc* loop = &optLoopTable[lnum]; - /* Do we already know what variables are assigned within this loop? */ - + // Do we already know what variables are assigned within this loop? + // if (!(loop->lpFlags & LPFLG_ASGVARS_YES)) { isVarAssgDsc desc; - /* Prepare the descriptor used by the tree walker call-back */ - + // Prepare the descriptor used by the tree walker call-back + // desc.ivaVar = (unsigned)-1; desc.ivaSkip = nullptr; #ifdef DEBUG @@ -5778,8 +5907,8 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi desc.ivaMaskCall = CALLINT_NONE; desc.ivaMaskIncomplete = false; - /* Now walk all the statements of the loop */ - + // Now walk all the statements of the loop + // for (BasicBlock* const block : loop->LoopBlocks()) { for (Statement* const stmt : block->NonPhiStatements()) @@ -5797,15 +5926,16 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi loop->lpAsgInds = desc.ivaMaskInd; loop->lpAsgCall = desc.ivaMaskCall; - /* Now we know what variables are assigned in the loop */ - + // Now we know what variables are assigned in the loop + // loop->lpFlags |= LPFLG_ASGVARS_YES; } - /* Now we can finally test the caller's mask against the loop's */ + // Now we can finally test the caller's mask against the loop's + // if (!AllVarSetOps::IsEmptyIntersection(this, loop->lpAsgVars, vars) || (loop->lpAsgInds & inds)) { - return 1; + return true; } switch (loop->lpAsgCall) @@ -5816,7 +5946,7 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi if (loop->lpAsgInds != VR_NONE) { - return 1; + return true; } break; @@ -5827,7 +5957,7 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi if (loop->lpAsgInds & VR_IND_REF) { - return 1; + return true; } break; @@ -5838,7 +5968,7 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi if (loop->lpAsgInds & VR_IND_SCL) { - return 1; + return true; } break; @@ -5849,7 +5979,7 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi if (loop->lpAsgInds & (VR_IND_REF | VR_IND_SCL)) { - return 1; + return true; } break; @@ -5864,7 +5994,7 @@ int Compiler::optIsSetAssgLoop(unsigned lnum, ALLVARSET_VALARG_TP vars, varRefKi noway_assert(!"Unexpected lpAsgCall value"); } - return 0; + return false; } void Compiler::optPerformHoistExpr(GenTree* origExpr, BasicBlock* exprBb, unsigned lnum) diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_1.cs b/src/tests/JIT/opt/Cloning/Runtime_61040_1.cs new file mode 100644 index 00000000000000..a2a298d737f03c --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_1.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +struct ArrayWrapper +{ + public int[] Array; +} + +class Runtime_61040_1 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static void JitUse(T arg) { } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static void Problem(ArrayWrapper a) + { + a = GetArrayLong(); + + JitUse(a); + JitUse(a); + + for (int i = 0; i < 10000; i++) + { + a = GetArray(); + JitUse(a.Array[i]); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static ArrayWrapper GetArray() => new() { Array = new int[0] }; + + [MethodImpl(MethodImplOptions.NoInlining)] + static ArrayWrapper GetArrayLong() => new() { Array = new int[10000] }; + + public static int Main() + { + int result = -1; + try + { + Problem(default); + } + catch (IndexOutOfRangeException e) + { + Console.WriteLine(e.Message); + result = 100; + } + return result; + } +} + diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_1.csproj b/src/tests/JIT/opt/Cloning/Runtime_61040_1.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_1.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_2.cs b/src/tests/JIT/opt/Cloning/Runtime_61040_2.cs new file mode 100644 index 00000000000000..19c70b474f13a5 --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_2.cs @@ -0,0 +1,49 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +[StructLayout(LayoutKind.Explicit)] +struct StructWithHoles +{ + [FieldOffset(0)] + public int Index; + [FieldOffset(5)] + public byte B; + [FieldOffset(8)] + public int C; +} + +class Runtime_61040_2 +{ + static int z = 0; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void JitUse(int arg) { z += arg; } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static int Problem(StructWithHoles a, StructWithHoles b, int[] d) + { + var a1 = a; + var b1 = b; + + try + { + for (a1.Index = 0; a1.Index < 10; a1.Index = a1.Index + 1) + { + a1 = b1; + JitUse(d[a1.Index]); + } + } + catch (IndexOutOfRangeException) + { + return z + 100; + } + + return -1; + } + + public static int Main() => Problem(new() { Index = 0 }, new() { Index = 100_000_000 }, new int[10]); +} diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_2.csproj b/src/tests/JIT/opt/Cloning/Runtime_61040_2.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_2.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_3.cs b/src/tests/JIT/opt/Cloning/Runtime_61040_3.cs new file mode 100644 index 00000000000000..2fd71fff30730d --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_3.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. + +using System; +using System.Runtime.CompilerServices; + +struct StructWithIndex +{ + public int Index; + public int Value; +} + +class Runtime_61040_3 +{ + static int z = 100; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void JitUse(int arg) { z++; } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static int Problem(StructWithIndex a, int[] d) + { + var a1 = a; + + try + { + for (a1.Index = 0; a1.Index < 10; a1.Index = a1.Index + 1) + { + a1 = GetStructWithIndex(); + JitUse(d[a1.Index]); + } + } + catch (IndexOutOfRangeException) + { + return z; + } + + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static StructWithIndex GetStructWithIndex() => new() { Index = 100_000_000 }; + + public static int Main() => Problem(new() { Index = 0, Value = 33 }, new int[10]); +} diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_3.csproj b/src/tests/JIT/opt/Cloning/Runtime_61040_3.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_3.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_4.cs b/src/tests/JIT/opt/Cloning/Runtime_61040_4.cs new file mode 100644 index 00000000000000..85b41f33273dc7 --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_4.cs @@ -0,0 +1,49 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +class Runtime_61040_4 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static void JitUse(T arg) { } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static int Problem() + { + int[] a = GetArray(); + int[] b = a; + int[] c = GetArray(); + + JitUse(a); + JitUse(b); + JitUse(c); + + int r = 0; + + try + { + for (int i = 0; i < a.Length; i++) + { + a = GetArrayLong(); + r += b[i]; + } + } + catch (IndexOutOfRangeException) + { + return r; + } + + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int[] GetArray() => new int[] { 1, 2, 3, 4, 90 }; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int[] GetArrayLong() => new int[10000]; + + public static int Main() => Problem(); +} + diff --git a/src/tests/JIT/opt/Cloning/Runtime_61040_4.csproj b/src/tests/JIT/opt/Cloning/Runtime_61040_4.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/opt/Cloning/Runtime_61040_4.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From 68191e838b44e2df669ce3ee37dcf2bb9c6e25e5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 2 Jun 2022 08:50:10 -0700 Subject: [PATCH 2/3] review feedback --- src/coreclr/jit/optimizer.cpp | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 1a748c2d4984c7..e9d4f4de5320de 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -932,23 +932,32 @@ bool Compiler::optCheckIterInLoopTest(unsigned loopInd, GenTree* test, unsigned { optLoopTable[loopInd].lpFlags |= LPFLG_ARRLEN_LIMIT; } + else + { + JITDUMP("Array limit var %V02u modifiable in " FMT_LP "\n", array->AsLclVarCommon()->GetLclNum(), + loopInd); + } + } + else + { + JITDUMP("Array limit tree [%06u] not analyzable in " FMT_LP "\n", dspTreeID(limitOp), loopInd); } } else { - // Not clear why we return false here. We know the loop exit test - // is comparing iterVar to something; we just don't know what. - // - // No real difference between this case and the cases above where - // we don't set some kind of limit flag. - // - return false; + JITDUMP("Loop limit tree [%06u] not analyzable in " FMT_LP "\n", dspTreeID(limitOp), loopInd); } + // Were we able to successfully analyze the limit? + // + const bool analyzedLimit = + (optLoopTable[loopInd].lpFlags & (LPFLG_CONST_LIMIT | LPFLG_VAR_LIMIT | LPFLG_ARRLEN_LIMIT)) != 0; + // Save the type of the comparison between the iterator and the limit. // optLoopTable[loopInd].lpTestTree = relop; - return true; + + return analyzedLimit; } //---------------------------------------------------------------------------------- @@ -1001,8 +1010,8 @@ unsigned Compiler::optIsLoopIncrTree(GenTree* incr) // optComputeIterInfo: Check tree is loop increment of a lcl that is loop-invariant. // // Arguments: -// lnum - loop in question // incr - tree that increments the loop iterator. v+=1 or v=v+1. +// from, to - range of blocks that comprise the loop body // pIterVar - see return value. // // Return Value: @@ -5824,7 +5833,7 @@ bool Compiler::optIsVarAssgLoop(unsigned lnum, unsigned var) LclVarDsc* const varDsc = lvaGetDesc(var); if (varDsc->IsAddressExposed()) { - // Assume the worst + // Assume the worst (that var is possibly modified in the loop) // return true; } @@ -5850,7 +5859,7 @@ bool Compiler::optIsVarAssgLoop(unsigned lnum, unsigned var) { // Parent var index is too large, assume the worst. // - return false; + return true; } } @@ -5860,7 +5869,7 @@ bool Compiler::optIsVarAssgLoop(unsigned lnum, unsigned var) { if (varDsc->lvIsStructField) { - return false; + return true; } return optIsVarAssigned(optLoopTable[lnum].lpHead->bbNext, optLoopTable[lnum].lpBottom, nullptr, var); From 2ddd0042ff86b619307c48150b48780c6ca9ab4c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 2 Jun 2022 12:22:22 -0700 Subject: [PATCH 3/3] handle more def cases --- src/coreclr/jit/optimizer.cpp | 115 +++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 52 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index e9d4f4de5320de..242a79e5b25e67 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5686,74 +5686,85 @@ bool Compiler::optNarrowTree(GenTree* tree, var_types srct, var_types dstt, Valu // Compiler::fgWalkResult Compiler::optIsVarAssgCB(GenTree** pTree, fgWalkData* data) { - GenTree* tree = *pTree; + GenTree* const tree = *pTree; + isVarAssgDsc* const desc = (isVarAssgDsc*)data->pCallbackData; + assert(desc && desc->ivaSelf == desc); - if (tree->OperIsSsaDef()) + // Can this tree define a local? + // + if (!tree->OperIsSsaDef()) { - isVarAssgDsc* desc = (isVarAssgDsc*)data->pCallbackData; - assert(desc && desc->ivaSelf == desc); - - GenTree* dest = nullptr; - if (tree->OperIs(GT_CALL)) - { - desc->ivaMaskCall = optCallInterf(tree->AsCall()); + return WALK_CONTINUE; + } - dest = data->compiler->gtCallGetDefinedRetBufLclAddr(tree->AsCall()); - if (dest == nullptr) - { - return WALK_CONTINUE; - } + // Check for calls and determine what's written. + // + GenTree* dest = nullptr; + if (tree->OperIs(GT_CALL)) + { + desc->ivaMaskCall = optCallInterf(tree->AsCall()); - dest = dest->AsOp()->gtOp1; - } - else + dest = data->compiler->gtCallGetDefinedRetBufLclAddr(tree->AsCall()); + if (dest == nullptr) { - dest = tree->AsOp()->gtOp1; + return WALK_CONTINUE; } - genTreeOps destOper = dest->OperGet(); + dest = dest->AsOp()->gtOp1; + } + else + { + dest = tree->AsOp()->gtOp1; + } - if (destOper == GT_LCL_VAR) - { - unsigned tvar = dest->AsLclVarCommon()->GetLclNum(); - if (tvar < lclMAX_ALLSET_TRACKED) - { - AllVarSetOps::AddElemD(data->compiler, desc->ivaMaskVal, tvar); - } - else - { - desc->ivaMaskIncomplete = true; - } + genTreeOps const destOper = dest->OperGet(); - if (tvar == desc->ivaVar) - { - if (tree != desc->ivaSkip) - { - return WALK_ABORT; - } - } + // Determine if the tree modifies a particular local + // + GenTreeLclVarCommon* lcl = nullptr; + if (tree->DefinesLocal(data->compiler, &lcl)) + { + const unsigned lclNum = lcl->GetLclNum(); + + if (lclNum < lclMAX_ALLSET_TRACKED) + { + AllVarSetOps::AddElemD(data->compiler, desc->ivaMaskVal, lclNum); } - else if (destOper == GT_LCL_FLD) + else { - // We can't track every field of every var. Moreover, indirections - // may access different parts of the var as different (but - // overlapping) fields. So just treat them as indirect accesses - // - // unsigned lclNum = dest->AsLclFld()->GetLclNum(); - // noway_assert(lvaTable[lclNum].lvAddrTaken); - // - varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL; - desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs); + desc->ivaMaskIncomplete = true; } - else if (destOper == GT_IND) + + // Bail out if we were checking for one particular local + // and we now see it's modified (ignoring perhaps + // the one tree where we expect modifications). + // + if ((lclNum == desc->ivaVar) && (tree != desc->ivaSkip)) { - // Set the proper indirection bits - // - varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL; - desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs); + return WALK_ABORT; } } + if (destOper == GT_LCL_FLD) + { + // We can't track every field of every var. Moreover, indirections + // may access different parts of the var as different (but + // overlapping) fields. So just treat them as indirect accesses + // + // unsigned lclNum = dest->AsLclFld()->GetLclNum(); + // noway_assert(lvaTable[lclNum].lvAddrTaken); + // + varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL; + desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs); + } + else if (destOper == GT_IND) + { + // Set the proper indirection bits + // + varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL; + desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs); + } + return WALK_CONTINUE; }