diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index bf49e63ab0cbb..54c1e0961e8a6 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -8523,6 +8523,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX * cBlocks, dBlocks : Display all the basic blocks of a function (call fgDispBasicBlocks()). * cBlocksV, dBlocksV : Display all the basic blocks of a function (call fgDispBasicBlocks(true)). * "V" means "verbose", and will dump all the trees. + * cStmt, dStmt : Display a Statement (call gtDispStmt()). * cTree, dTree : Display a tree (call gtDispTree()). * cTreeLIR, dTreeLIR : Display a tree in LIR form (call gtDispLIRNode()). * cTrees, dTrees : Display all the trees in a function (call fgDumpTrees()). @@ -8545,6 +8546,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX * * The following don't require a Compiler* to work: * dRegMask : Display a regMaskTP (call dspRegMask(mask)). + * dBlockList : Display a BasicBlockList*. */ void cBlock(Compiler* comp, BasicBlock* block) @@ -8719,6 +8721,11 @@ void dBlocksV() cBlocksV(JitTls::GetCompiler()); } +void dStmt(Statement* statement) +{ + cStmt(JitTls::GetCompiler(), statement); +} + void dTree(GenTree* tree) { cTree(JitTls::GetCompiler(), tree); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b60353d8e8440..fb613d22f972c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3116,6 +3116,8 @@ class Compiler void gtUpdateNodeOperSideEffects(GenTree* tree); + void gtUpdateNodeOperSideEffectsPost(GenTree* tree); + // Returns "true" iff the complexity (not formally defined, but first interpretation // is #of nodes in subtree) of "tree" is greater than "limit". // (This is somewhat redundant with the "GetCostEx()/GetCostSz()" fields, but can be used diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 65934aad6adcd..9f7d52cb24c7b 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3253,14 +3253,13 @@ void Compiler::fgDebugCheckFlags(GenTree* tree) } //------------------------------------------------------------------------------ -// fgDebugCheckDispFlags: -// Wrapper function that displays two GTF_IND_ flags -// and then calls ftDispFlags to display the rest. +// fgDebugCheckDispFlags: Wrapper function that displays GTF_IND_ flags +// and then calls gtDispFlags to display the rest. // // Arguments: // tree - Tree whose flags are being checked -// dispFlags - the first argument for gtDispFlags -// ands hold GTF_IND_INVARIANT and GTF_IND_NONFLUALTING +// dispFlags - the first argument for gtDispFlags (flags to display), +// including GTF_IND_INVARIANT, GTF_IND_NONFAULTING, GTF_IND_NONNULL // debugFlags - the second argument to gtDispFlags // void Compiler::fgDebugCheckDispFlags(GenTree* tree, GenTreeFlags dispFlags, GenTreeDebugFlags debugFlags) @@ -3277,15 +3276,14 @@ void Compiler::fgDebugCheckDispFlags(GenTree* tree, GenTreeFlags dispFlags, GenT //------------------------------------------------------------------------------ // fgDebugCheckFlagsHelper : Check if all bits that are set in chkFlags are also set in treeFlags. // -// // Arguments: -// tree - Tree whose flags are being checked +// tree - Tree whose flags are being checked // treeFlags - Actual flags on the tree -// chkFlags - Expected flags +// chkFlags - Expected flags // // Note: // Checking that all bits that are set in treeFlags are also set in chkFlags is currently disabled. - +// void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags treeFlags, GenTreeFlags chkFlags) { if (chkFlags & ~treeFlags) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 1f3e71ade8ab0..2cc766532e4f6 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -933,13 +933,12 @@ GenTreeCall* Compiler::fgGetSharedCCtor(CORINFO_CLASS_HANDLE cls) //------------------------------------------------------------------------------ // fgAddrCouldBeNull : Check whether the address tree can represent null. // -// // Arguments: // addr - Address to check // // Return Value: // True if address could be null; false otherwise - +// bool Compiler::fgAddrCouldBeNull(GenTree* addr) { addr = addr->gtEffectiveVal(); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 7f16e6cd40048..c38aae0296474 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8533,7 +8533,7 @@ void Compiler::gtUpdateSideEffects(Statement* stmt, GenTree* tree) // // Arguments: // tree - Tree to update the side effects for - +// void Compiler::gtUpdateTreeAncestorsSideEffects(GenTree* tree) { assert(fgStmtListThreaded); @@ -8549,7 +8549,7 @@ void Compiler::gtUpdateTreeAncestorsSideEffects(GenTree* tree) // // Arguments: // stmt - The statement to update side effects on - +// void Compiler::gtUpdateStmtSideEffects(Statement* stmt) { fgWalkTree(stmt->GetRootNodePointer(), fgUpdateSideEffectsPre, fgUpdateSideEffectsPost); @@ -8565,7 +8565,7 @@ void Compiler::gtUpdateStmtSideEffects(Statement* stmt) // This method currently only updates GTF_EXCEPT, GTF_ASG, and GTF_CALL flags. // The other side effect flags may remain unnecessarily (conservatively) set. // The caller of this method is expected to update the flags based on the children's flags. - +// void Compiler::gtUpdateNodeOperSideEffects(GenTree* tree) { if (tree->OperMayThrow(this)) @@ -8600,6 +8600,38 @@ void Compiler::gtUpdateNodeOperSideEffects(GenTree* tree) } } +//------------------------------------------------------------------------ +// gtUpdateNodeOperSideEffectsPost: Update the side effects based on the node operation, +// in the post-order visit of a tree walk. It is expected that the pre-order visit cleared +// the bits, so the post-order visit only sets them. This is important for binary nodes +// where one child already may have set the GTF_EXCEPT bit. Note that `SetIndirExceptionFlags` +// looks at its child, which is why we need to do this in a bottom-up walk. +// +// Arguments: +// tree - Tree to update the side effects on +// +// Notes: +// This method currently only updates GTF_ASG, GTF_CALL, and GTF_EXCEPT flags. +// The other side effect flags may remain unnecessarily (conservatively) set. +// +void Compiler::gtUpdateNodeOperSideEffectsPost(GenTree* tree) +{ + if (tree->OperMayThrow(this)) + { + tree->gtFlags |= GTF_EXCEPT; + } + + if (tree->OperRequiresAsgFlag()) + { + tree->gtFlags |= GTF_ASG; + } + + if (tree->OperRequiresCallFlag(this)) + { + tree->gtFlags |= GTF_CALL; + } +} + //------------------------------------------------------------------------ // gtUpdateNodeSideEffects: Update the side effects based on the node operation and // children's side efects. @@ -8608,9 +8640,9 @@ void Compiler::gtUpdateNodeOperSideEffects(GenTree* tree) // tree - Tree to update the side effects on // // Notes: -// This method currently only updates GTF_EXCEPT and GTF_ASG flags. The other side effect -// flags may remain unnecessarily (conservatively) set. - +// This method currently only updates GTF_EXCEPT, GTF_ASG, and GTF_CALL flags. +// The other side effect flags may remain unnecessarily (conservatively) set. +// void Compiler::gtUpdateNodeSideEffects(GenTree* tree) { gtUpdateNodeOperSideEffects(tree); @@ -8627,24 +8659,23 @@ void Compiler::gtUpdateNodeSideEffects(GenTree* tree) //------------------------------------------------------------------------ // fgUpdateSideEffectsPre: Update the side effects based on the tree operation. +// The pre-visit walk clears GTF_ASG, GTF_CALL, and GTF_EXCEPT; the post-visit walk sets +// the bits as necessary. // // Arguments: // pTree - Pointer to the tree to update the side effects // fgWalkPre - Walk data // -// Notes: -// This method currently only updates GTF_EXCEPT and GTF_ASG flags. The other side effect -// flags may remain unnecessarily (conservatively) set. - Compiler::fgWalkResult Compiler::fgUpdateSideEffectsPre(GenTree** pTree, fgWalkData* fgWalkPre) { - fgWalkPre->compiler->gtUpdateNodeOperSideEffects(*pTree); + GenTree* tree = *pTree; + tree->gtFlags &= ~(GTF_ASG | GTF_CALL | GTF_EXCEPT); return WALK_CONTINUE; } //------------------------------------------------------------------------ -// fgUpdateSideEffectsPost: Update the side effects of the parent based on the tree's flags. +// fgUpdateSideEffectsPost: Update the side effects of the node and parent based on the tree's flags. // // Arguments: // pTree - Pointer to the tree @@ -8653,10 +8684,23 @@ Compiler::fgWalkResult Compiler::fgUpdateSideEffectsPre(GenTree** pTree, fgWalkD // Notes: // The routine is used for updating the stale side effect flags for ancestor // nodes starting from treeParent up to the top-level stmt expr. - +// Compiler::fgWalkResult Compiler::fgUpdateSideEffectsPost(GenTree** pTree, fgWalkData* fgWalkPost) { - GenTree* tree = *pTree; + GenTree* tree = *pTree; + + // Update the node's side effects first. + fgWalkPost->compiler->gtUpdateNodeOperSideEffectsPost(tree); + + // If this node is an indir or array length, and it doesn't have the GTF_EXCEPT bit set, we + // set the GTF_IND_NONFAULTING bit. This needs to be done after all children, and this node, have + // been processed. + if (tree->OperIsIndirOrArrLength() && ((tree->gtFlags & GTF_EXCEPT) == 0)) + { + tree->gtFlags |= GTF_IND_NONFAULTING; + } + + // Then update the parent's side effects based on this node. GenTree* parent = fgWalkPost->parent; if (parent != nullptr) { @@ -9908,9 +9952,16 @@ bool GenTree::Precedes(GenTree* other) // Arguments: // comp - compiler instance // - void GenTree::SetIndirExceptionFlags(Compiler* comp) { + assert(OperIsIndirOrArrLength()); + + if (OperMayThrow(comp)) + { + gtFlags |= GTF_EXCEPT; + return; + } + GenTree* addr = nullptr; if (OperIsIndir()) { @@ -9922,7 +9973,7 @@ void GenTree::SetIndirExceptionFlags(Compiler* comp) addr = AsArrLen()->ArrRef(); } - if (OperMayThrow(comp) || ((addr->gtFlags & GTF_EXCEPT) != 0)) + if ((addr->gtFlags & GTF_EXCEPT) != 0) { gtFlags |= GTF_EXCEPT; } diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp index c3991e663e1d9..69e916c324952 100644 --- a/src/coreclr/jit/loopcloning.cpp +++ b/src/coreclr/jit/loopcloning.cpp @@ -2074,9 +2074,15 @@ bool Compiler::optIsStackLocalInvariant(unsigned loopNum, unsigned lclNum) // dimension of [] encountered. // // Operation: -// Given a "tree" extract the GT_INDEX node in "result" as ArrIndex. In FlowGraph morph -// we have converted a GT_INDEX tree into a scaled index base offset expression. We need -// to reconstruct this to be able to know if this is an array access. +// Given a "tree" extract the GT_INDEX node in "result" as ArrIndex. In morph +// we have converted a GT_INDEX tree into a scaled index base offset expression. +// However, we don't actually bother to parse the morphed tree. All we care about is +// the bounds check node: it contains the array base and element index. The other side +// of the COMMA node can vary between array of primitive type and array of struct. There's +// no need to parse that, as the array bounds check contains the only thing we care about. +// In particular, we are trying to find bounds checks to remove, so only looking at the bounds +// check makes sense. We could verify that the bounds check is against the same array base/index +// but it isn't necessary. // // Assumption: // The method extracts only if the array base and indices are GT_LCL_VAR. @@ -2115,6 +2121,12 @@ bool Compiler::optIsStackLocalInvariant(unsigned loopNum, unsigned lclNum) // | \--* LCL_VAR int V03 loc2 // \--* CNS_INT long 16 Fseq[#FirstElem] // +// The COMMA op2 expression is the array index expression (or SIMD/Span expression). If we've got +// a "LCL_VAR int" index and "ARR_LENGTH(LCL_VAR ref)", that's good enough for us: we'll assume +// op2 is an array index expression. We don't need to match it just to ensure the index var is +// used as an index expression, or array base var is used as the array base. This saves us from parsing +// all the forms that morph can create, especially for arrays of structs. +// bool Compiler::optExtractArrIndex(GenTree* tree, ArrIndex* result, unsigned lhsNum) { if (tree->gtOper != GT_COMMA) @@ -2150,72 +2162,6 @@ bool Compiler::optExtractArrIndex(GenTree* tree, ArrIndex* result, unsigned lhsN unsigned indLcl = arrBndsChk->gtIndex->AsLclVarCommon()->GetLclNum(); - GenTree* after = tree->gtGetOp2(); - - if (after->gtOper != GT_IND) - { - return false; - } - // It used to be the case that arrBndsChks for struct types would fail the previous check because - // after->gtOper was an address (for a block op). In order to avoid asmDiffs we will for now - // return false if the type of 'after' is a struct type. (This was causing us to clone loops - // that we were not previously cloning.) - // TODO-1stClassStructs: Remove this check to enable optimization of array bounds checks for struct - // types. - if (varTypeIsStruct(after)) - { - return false; - } - - GenTree* sibo = after->gtGetOp1(); // sibo = scale*index + base + offset - if (sibo->gtOper != GT_ADD) - { - return false; - } - GenTree* base = sibo->gtGetOp1(); - GenTree* sio = sibo->gtGetOp2(); // sio == scale*index + offset - if (base->OperGet() != GT_LCL_VAR || base->AsLclVarCommon()->GetLclNum() != arrLcl) - { - return false; - } - if (sio->gtOper != GT_ADD) - { - return false; - } - GenTree* ofs = sio->gtGetOp2(); - GenTree* si = sio->gtGetOp1(); // si = scale*index - if (ofs->gtOper != GT_CNS_INT) - { - return false; - } - GenTree* index; - if (si->gtOper == GT_LSH) - { - GenTree* scale = si->gtGetOp2(); - index = si->gtGetOp1(); - if (scale->gtOper != GT_CNS_INT) - { - return false; - } - } - else - { - // No scale (e.g., byte array). - index = si; - } -#ifdef TARGET_64BIT - if (index->gtOper != GT_CAST) - { - return false; - } - GenTree* indexVar = index->gtGetOp1(); -#else - GenTree* indexVar = index; -#endif - if (indexVar->gtOper != GT_LCL_VAR || indexVar->AsLclVarCommon()->GetLclNum() != indLcl) - { - return false; - } if (lhsNum == BAD_VAR_NUM) { result->arrLcl = arrLcl; @@ -2241,26 +2187,13 @@ bool Compiler::optExtractArrIndex(GenTree* tree, ArrIndex* result, unsigned lhsN // "result" contains the array access depth. The "indLcls" fields contain the indices. // // Operation: -// Recursively look for a list of array indices. In the example below, we encounter, -// V03 = ((V05 = V00[V01]), (V05[V02])) which corresponds to access of V00[V01][V02] -// The return value would then be: +// Recursively look for a list of array indices. For example, if the tree is +// V03 = (V05 = V00[V01]), V05[V02] +// that corresponds to access of V00[V01][V02]. The return value would then be: // ArrIndex result { arrLcl: V00, indLcls: [V01, V02], rank: 2 } // -// V00[V01][V02] would be morphed as: -// -// [000000001B366848] ---XG------- indir int -// [000000001B36BC50] ------------ V05 + (V02 << 2) + 16 -// [000000001B36C200] ---XG------- comma int -// [000000001B36BDB8] ---X-------- arrBndsChk(V05, V02) -// [000000001B36C278] -A-XG------- comma int -// [000000001B366730] R--XG------- indir ref -// [000000001B36C2F0] ------------ V00 + (V01 << 3) + 24 -// [000000001B36C818] ---XG------- comma ref -// [000000001B36C458] ---X-------- arrBndsChk(V00, V01) -// [000000001B36BB60] -A-XG------- = ref -// [000000001B36BAE8] D------N---- lclVar ref V05 tmp2 -// [000000001B36A668] -A-XG------- = int -// [000000001B36A5F0] D------N---- lclVar int V03 tmp0 +// Note that the array expression is implied by the array bounds check under the COMMA, and the array bounds +// checks is what is parsed from the morphed tree; the array addressing expression is not parsed. // // Assumption: // The method extracts only if the array base and indices are GT_LCL_VAR. diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 354b1e4a35749..000103b54c5b0 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5440,7 +5440,7 @@ BasicBlock* Compiler::fgSetRngChkTargetInner(SpecialCodeKind kind, bool delay) * The orginal GT_INDEX node is bashed into the GT_IND node that accesses * the array element. We expand the GT_INDEX node into a larger tree that * evaluates the array base and index. The simplest expansion is a GT_COMMA - * with a GT_ARR_BOUND_CHK and a GT_IND with a GTF_INX_RNGCHK flag. + * with a GT_ARR_BOUNDS_CHECK and a GT_IND with a GTF_INX_RNGCHK flag. * For complex array or index expressions one or more GT_COMMA assignments * are inserted so that we only evaluate the array or index expressions once. * @@ -5530,7 +5530,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) // side-effecting. // 2. Evaluate the array index expression and store the result in a temp if the expression is complex or // side-effecting. - // 3. Perform an explicit bounds check: GT_ARR_BOUNDS_CHK(index, GT_ARR_LENGTH(array)) + // 3. Perform an explicit bounds check: GT_ARR_BOUNDS_CHECK(index, GT_ARR_LENGTH(array)) // 4. Compute the address of the element that will be accessed: // GT_ADD(GT_ADD(array, firstElementOffset), GT_MUL(index, elementSize)) // 5. Dereference the address with a GT_IND. @@ -5716,9 +5716,6 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) // play) to create a "partial" byref that doesn't point exactly to the correct object; there is risk that // the partial byref will not point within the object, and thus not get updated correctly during a GC. // This is mostly a risk in fully-interruptible code regions. - // - // NOTE: the tree form created here is pattern matched by optExtractArrIndex(), so changes here must - // be reflected there. /* Add the first element's offset */ @@ -5798,11 +5795,17 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), arrRefDefn, tree); } + JITDUMP("fgMorphArrayIndex (before remorph):\n"); + DISPTREE(tree); + // Currently we morph the tree to perform some folding operations prior // to attaching fieldSeq info and labeling constant array index contributions // fgMorphTree(tree); + JITDUMP("fgMorphArrayIndex (after remorph):\n"); + DISPTREE(tree); + // Ideally we just want to proceed to attaching fieldSeq info and labeling the // constant array index contributions, but the morphing operation may have changed // the 'tree' into something that now unconditionally throws an exception. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 748d5feabb562..24985c721c851 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -7417,8 +7417,10 @@ GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma, #ifdef DEBUG if (verbose) { - printf("After optRemoveRangeCheck:\n"); - gtDispTree(tree); + // gtUpdateSideEffects can update the side effects for ancestors in the tree, so display the whole statement + // tree, not just the sub-tree. + printf("After optRemoveRangeCheck for [%06u]:\n", dspTreeID(tree)); + gtDispTree(stmt->GetRootNode()); } #endif