diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 7f9f38da566b92..2bbd1a5ea04052 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16237,7 +16237,6 @@ bool Compiler::gtSplitTree( { if (*use == m_splitNode) { - bool userIsLocation = false; bool userIsReturned = false; // Split all siblings and ancestor siblings. int i; @@ -16254,18 +16253,16 @@ bool Compiler::gtSplitTree( // that contains the split node. if (m_useStack.BottomRef(i + 1).User == useInf.User) { - SplitOutUse(useInf, userIsLocation, userIsReturned); + SplitOutUse(useInf, userIsReturned); } else { // This is an ancestor of the node we're splitting on. - userIsLocation = IsLocation(useInf, userIsLocation); userIsReturned = IsReturned(useInf, userIsReturned); } } assert(m_useStack.Bottom(i).Use == use); - userIsLocation = IsLocation(m_useStack.BottomRef(i), userIsLocation); userIsReturned = IsReturned(m_useStack.BottomRef(i), userIsReturned); // The remaining nodes should be operands of the split node. @@ -16273,7 +16270,7 @@ bool Compiler::gtSplitTree( { const UseInfo& useInf = m_useStack.BottomRef(i); assert(useInf.User == *use); - SplitOutUse(useInf, userIsLocation, userIsReturned); + SplitOutUse(useInf, userIsReturned); } SplitNodeUse = use; @@ -16290,25 +16287,22 @@ bool Compiler::gtSplitTree( } private: - bool IsLocation(const UseInfo& useInf, bool userIsLocation) + bool IsLocation(const UseInfo& useInf) { - if (useInf.User != nullptr) + if (useInf.User == nullptr) { - if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1)) - { - return true; - } + return false; + } - if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) && - (useInf.Use == &useInf.User->AsStoreDynBlk()->Data())) - { - return true; - } + if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1)) + { + return true; + } - if (userIsLocation && useInf.User->OperIs(GT_COMMA) && (useInf.Use == &useInf.User->AsOp()->gtOp2)) - { - return true; - } + if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) && + (useInf.Use == &useInf.User->AsStoreDynBlk()->Data())) + { + return true; } return false; @@ -16332,7 +16326,7 @@ bool Compiler::gtSplitTree( return false; } - void SplitOutUse(const UseInfo& useInf, bool userIsLocation, bool userIsReturned) + void SplitOutUse(const UseInfo& useInf, bool userIsReturned) { GenTree** use = useInf.Use; GenTree* user = useInf.User; @@ -16363,52 +16357,13 @@ bool Compiler::gtSplitTree( return; } - if (IsLocation(useInf, userIsLocation)) + if (IsLocation(useInf)) { - if ((*use)->OperIs(GT_COMMA)) - { - // We have: - // User - // COMMA - // op1 - // op2 - // rhs - // where the comma is a location, and we want to split it out. - // - // The first use will be the COMMA --- op1 edge, which we - // expect to be handled by simple side effect extraction in - // the recursive call. - UseInfo use1{&(*use)->AsOp()->gtOp1, *use}; - - // For the second use we will update the user to use op2 - // directly instead of the comma so that we get the proper - // location treatment. The edge will then be the User --- - // op2 edge. - *use = (*use)->gtGetOp2(); - MadeChanges = true; - - UseInfo use2{use, user}; - - // Locations are never returned. - assert(!IsReturned(useInf, userIsReturned)); - if ((*use)->IsReverseOp()) - { - SplitOutUse(use2, true, false); - SplitOutUse(use1, false, false); - } - else - { - SplitOutUse(use1, false, false); - SplitOutUse(use2, true, false); - } - return; - } - // Only a handful of nodes can be location, and they are all unary or nullary. assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD)); if ((*use)->OperIsUnary()) { - SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false, false); + SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false); } return; @@ -16421,7 +16376,7 @@ bool Compiler::gtSplitTree( if ((user != nullptr) && user->OperIs(GT_MUL) && user->Is64RsltMul()) { assert((*use)->OperIs(GT_CAST)); - SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false, false); + SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false); return; } #endif @@ -16430,7 +16385,7 @@ bool Compiler::gtSplitTree( { for (GenTree** operandUse : (*use)->UseEdges()) { - SplitOutUse(UseInfo{operandUse, *use}, false, false); + SplitOutUse(UseInfo{operandUse, *use}, false); } return; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index c623f9d10ec359..e43bc69a5752aa 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9827,12 +9827,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA // Only do this optimization when we are in the global optimizer. Doing this after value numbering // could result in an invalid value number for the newly generated GT_IND node. - if ((op1->OperGet() == GT_COMMA) && fgGlobalMorph) + // We skip INDs with GTF_DONT_CSE which is set if the IND is a location. + if (op1->OperIs(GT_COMMA) && fgGlobalMorph && ((tree->gtFlags & GTF_DONT_CSE) == 0)) { // Perform the transform IND(COMMA(x, ..., z)) -> COMMA(x, ..., IND(z)). - // TBD: this transformation is currently necessary for correctness -- it might - // be good to analyze the failures that result if we don't do this, and fix them - // in other ways. Ideally, this should be optional. GenTree* commaNode = op1; GenTreeFlags treeFlags = tree->gtFlags; commaNode->gtType = typ; @@ -11504,6 +11502,13 @@ GenTree* Compiler::fgPropagateCommaThrow(GenTree* parent, GenTreeOp* commaThrow, assert(fgGlobalMorph); assert(fgIsCommaThrow(commaThrow)); + bool mightBeLocation = parent->OperIsIndir() && ((parent->gtFlags & GTF_DONT_CSE) != 0); + + if (mightBeLocation) + { + return nullptr; + } + if ((commaThrow->gtFlags & GTF_COLON_COND) == 0) { fgRemoveRestOfBlock = true; diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index dd710070367565..2912b19d7f1409 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -163,12 +163,10 @@ GenTree* MorphInitBlockHelper::Morph() // void MorphInitBlockHelper::PrepareDst() { - GenTree* origDst = m_asg->gtGetOp1(); - m_dst = MorphBlock(m_comp, origDst, true); - if (m_dst != origDst) - { - m_asg->gtOp1 = m_dst; - } + m_dst = m_asg->gtGetOp1(); + + // Commas cannot be destinations. + assert(!m_dst->OperIs(GT_COMMA)); if (m_asg->TypeGet() != m_dst->TypeGet()) { @@ -213,7 +211,7 @@ void MorphInitBlockHelper::PrepareDst() #if defined(DEBUG) if (m_comp->verbose) { - printf("PrepareDst for [%06u] ", m_comp->dspTreeID(origDst)); + printf("PrepareDst for [%06u] ", m_comp->dspTreeID(m_dst)); if (m_dstLclNode != nullptr) { printf("have found a local var V%02u.\n", m_dstLclNum); @@ -1595,6 +1593,12 @@ GenTree* Compiler::fgMorphInitBlock(GenTree* tree) // GenTree* Compiler::fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree) { + if (!tree->Data()->OperIs(GT_CNS_INT, GT_INIT_VAL)) + { + // Data is a location and required to have GTF_DONT_CSE. + tree->Data()->gtFlags |= GTF_DONT_CSE; + } + tree->Addr() = fgMorphTree(tree->Addr()); tree->Data() = fgMorphTree(tree->Data()); tree->gtDynamicSize = fgMorphTree(tree->gtDynamicSize); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 8cb875e2548f1e..6394c8e53c011b 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -8617,9 +8617,11 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) if (oper == GT_ASG) { - GenTree* lhs = tree->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true); + GenTree* lhs = tree->gtGetOp1(); - if (lhs->OperGet() == GT_IND) + assert(!lhs->OperIs(GT_COMMA)); + + if (lhs->OperIs(GT_IND)) { GenTree* arg = lhs->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 037dae59e48175..1c9e7460285ed8 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9839,9 +9839,8 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) } } - // We have to handle the case where the LHS is a comma. In that case, we don't evaluate the comma, - // and we're really just interested in the effective value. - lhs = lhs->gtEffectiveVal(); + // Locations are not allowed to be COMMAs. + assert(!lhs->OperIs(GT_COMMA)); // Now, record the new VN for an assignment (performing the indicated "state update"). // It's safe to use gtEffectiveVal here, because the non-last elements of a comma list on the