From 9529803ae29c2804880c6bd8ca710b8c037cb498 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 30 Jan 2023 15:40:17 +0100 Subject: [PATCH] JIT: Avoid creating stores to dead fields in block morphing (#81095) Fix #80498 --- src/coreclr/jit/morphblock.cpp | 132 ++++++++++++++++++++++++++--------------- 1 file changed, 83 insertions(+), 49 deletions(-) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 1b92714..4b4ae27 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -542,10 +542,17 @@ void MorphInitBlockHelper::TryInitFieldByField() for (unsigned i = 0; i < destLclVar->lvFieldCnt; ++i) { - unsigned fieldLclNum = destLclVar->lvFieldLclStart + i; - LclVarDsc* fieldDesc = m_comp->lvaGetDesc(fieldLclNum); - var_types fieldType = fieldDesc->TypeGet(); - GenTree* dest = m_comp->gtNewLclvNode(fieldLclNum, fieldType); + unsigned fieldLclNum = destLclVar->lvFieldLclStart + i; + + if (m_comp->fgGlobalMorph && m_dstLclNode->IsLastUse(i)) + { + JITDUMP("Field-by-field init skipping write to dead field V%02u\n", fieldLclNum); + continue; + } + + LclVarDsc* fieldDesc = m_comp->lvaGetDesc(fieldLclNum); + var_types fieldType = fieldDesc->TypeGet(); + GenTree* dest = m_comp->gtNewLclvNode(fieldLclNum, fieldType); GenTree* src; switch (fieldType) @@ -610,6 +617,11 @@ void MorphInitBlockHelper::TryInitFieldByField() } } + if (tree == nullptr) + { + tree = m_comp->gtNewNothingNode(); + } + m_result = tree; m_transformationDecision = BlockTransformation::FieldByField; } @@ -1159,7 +1171,7 @@ void MorphCopyBlockHelper::TryPrimitiveCopy() // GenTree* MorphCopyBlockHelper::CopyFieldByField() { - GenTreeOp* asgFields = nullptr; + GenTree* result = nullptr; GenTree* dstAddr = nullptr; GenTree* srcAddr = nullptr; @@ -1170,6 +1182,19 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() unsigned fieldCnt = 0; + unsigned dyingFieldCnt = 0; + if (m_dstDoFldAsg) + { + fieldCnt = m_dstVarDsc->lvFieldCnt; + + if (m_comp->fgGlobalMorph) + { + dyingFieldCnt = + genCountBits(static_cast(m_dstLclNode->gtFlags & m_dstVarDsc->AllFieldDeathFlags())); + assert(dyingFieldCnt <= fieldCnt); + } + } + if (m_dstDoFldAsg && m_srcDoFldAsg) { // To do fieldwise assignments for both sides. @@ -1177,34 +1202,27 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() // at the same offsets. assert(m_dstLclNum != BAD_VAR_NUM && m_srcLclNum != BAD_VAR_NUM); assert(m_dstVarDsc != nullptr && m_srcVarDsc != nullptr && m_dstVarDsc->lvFieldCnt == m_srcVarDsc->lvFieldCnt); - - fieldCnt = m_dstVarDsc->lvFieldCnt; } else if (m_dstDoFldAsg) { - fieldCnt = m_dstVarDsc->lvFieldCnt; m_srcUseLclFld = m_srcVarDsc != nullptr; if (!m_srcUseLclFld) { srcAddr = m_src->AsIndir()->Addr(); - if (m_comp->gtClone(srcAddr)) + // "srcAddr" might be a complex expression that we need to clone + // and spill, unless we only end up using the address once. + if (fieldCnt - dyingFieldCnt > 1) { - // "srcAddr" is simple expression. No need to spill. - noway_assert((srcAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0); - } - else - { - // "srcAddr" is complex expression. Clone and spill it (unless the destination is - // a struct local that only has one field, in which case we'd only use the - // address value once...) - if (m_dstVarDsc->lvFieldCnt > 1) + if (m_comp->gtClone(srcAddr)) { - // We will spill "srcAddr" (i.e. assign to a temp "BlockOp address local") - // no need to clone a new copy as it is only used once - // - addrSpill = srcAddr; // addrSpill represents the "srcAddr" + // "srcAddr" is simple expression. No need to spill. + noway_assert((srcAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0); + } + else + { + addrSpill = srcAddr; } } } @@ -1225,26 +1243,38 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() { dstAddr = m_dst->AsIndir()->Addr(); - if (m_comp->gtClone(dstAddr)) - { - // "dstAddr" is simple expression. No need to spill - noway_assert((dstAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0); - } - else + // "dstAddr" might be a complex expression that we need to clone + // and spill, unless we only end up using the address once. + if (m_srcVarDsc->lvFieldCnt > 1) { - // "dstAddr" is complex expression. Clone and spill it (unless the source is a struct - // local that only has one field, in which case we'd only use the address value once...) - if (m_srcVarDsc->lvFieldCnt > 1) + if (m_comp->gtClone(dstAddr)) + { + // "dstAddr" is simple expression. No need to spill + noway_assert((dstAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0); + } + else { - // We will spill "dstAddr" (i.e. assign to a temp "BlockOp address local") - // no need to clone a new copy as it is only used once - // - addrSpill = dstAddr; // addrSpill represents the "dstAddr" + addrSpill = dstAddr; } } } } + if (dyingFieldCnt == fieldCnt) + { + JITDUMP("All fields of destination of field-by-field copy are dying, skipping entirely\n"); + + if (m_srcUseLclFld) + { + return m_comp->gtNewNothingNode(); + } + else + { + JITDUMP(" ...but keeping a nullcheck\n"); + return m_comp->gtNewIndir(TYP_BYTE, srcAddr); + } + } + if (addrSpill != nullptr) { // 'addrSpill' is already morphed @@ -1262,23 +1292,26 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() // We may have allocated a temp above, and that may have caused the lvaTable to be expanded. // So, beyond this point we cannot rely on the old values of 'm_srcVarDsc' and 'm_dstVarDsc'. + for (unsigned i = 0; i < fieldCnt; ++i) { GenTree* dstFld; if (m_dstDoFldAsg) { noway_assert((m_dstLclNum != BAD_VAR_NUM) && (dstAddr == nullptr)); + unsigned dstFieldLclNum = m_comp->lvaGetDesc(m_dstLclNum)->lvFieldLclStart + i; + if (m_comp->fgGlobalMorph && m_dstLclNode->IsLastUse(i)) + { + JITDUMP("Field-by-field copy skipping write to dead field V%02u\n", dstFieldLclNum); + continue; + } + dstFld = m_comp->gtNewLclvNode(dstFieldLclNum, m_comp->lvaGetDesc(dstFieldLclNum)->TypeGet()); // If it had been labeled a "USEASG", assignments to the individual promoted fields are not. dstFld->gtFlags |= m_dstLclNode->gtFlags & ~(GTF_NODE_MASK | GTF_VAR_USEASG | GTF_VAR_DEATH_MASK); - if (m_dstLclNode->IsLastUse(i)) - { - dstFld->gtFlags |= GTF_VAR_DEATH; - } - // Don't CSE the lhs of an assignment. dstFld->gtFlags |= GTF_DONT_CSE; } @@ -1307,9 +1340,9 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() } else { - if (i == (fieldCnt - 1)) + if (result == nullptr) { - // Reuse the original "dstAddr" tree for the last field. + // Reuse the original "dstAddr" tree for the first field. dstAddrClone = dstAddr; } else @@ -1395,9 +1428,9 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() } else { - if (i == (fieldCnt - 1)) + if (result == nullptr) { - // Reuse the original m_srcAddr tree for the last field. + // Reuse the original m_srcAddr tree for the first field. srcAddrClone = srcAddr; } else @@ -1482,19 +1515,20 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() if (addrSpillAsg != nullptr) { - asgFields = m_comp->gtNewOperNode(GT_COMMA, TYP_VOID, addrSpillAsg, asgOneFld)->AsOp(); + result = m_comp->gtNewOperNode(GT_COMMA, TYP_VOID, addrSpillAsg, asgOneFld)->AsOp(); addrSpillAsg = nullptr; } - else if (asgFields != nullptr) + else if (result != nullptr) { - asgFields = m_comp->gtNewOperNode(GT_COMMA, TYP_VOID, asgFields, asgOneFld)->AsOp(); + result = m_comp->gtNewOperNode(GT_COMMA, TYP_VOID, result, asgOneFld)->AsOp(); } else { - asgFields = asgOneFld; + result = asgOneFld; } } - return asgFields; + + return result; } //------------------------------------------------------------------------ -- 2.7.4