From 455551b2ff8f854a3e05a0c1eeb86c71dc849f8f Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Sat, 22 Jan 2022 02:24:10 +0300 Subject: [PATCH] Delete `GT_DYN_BLK` (#63026) * Import GT_STORE_DYN_BLK directly * Delete GT_DYN_BLK * DynBlk -> StoreDynBlk * Add some tests * Mark tests Pri-1 * Rebase and fix build * Bring back the odd early return --- src/coreclr/jit/assertionprop.cpp | 3 +- src/coreclr/jit/codegenlinear.cpp | 4 +- src/coreclr/jit/compiler.cpp | 3 +- src/coreclr/jit/compiler.h | 31 +--- src/coreclr/jit/compiler.hpp | 13 +- src/coreclr/jit/flowgraph.cpp | 32 +--- src/coreclr/jit/gentree.cpp | 167 +++++------------- src/coreclr/jit/gentree.h | 74 +++----- src/coreclr/jit/gtlist.h | 3 +- src/coreclr/jit/gtstructs.h | 6 +- src/coreclr/jit/importer.cpp | 45 +++-- src/coreclr/jit/lclmorph.cpp | 31 +--- src/coreclr/jit/liveness.cpp | 1 - src/coreclr/jit/lsraarm64.cpp | 1 - src/coreclr/jit/lsraarmarch.cpp | 2 +- src/coreclr/jit/lsraxarch.cpp | 3 +- src/coreclr/jit/morph.cpp | 19 +- src/coreclr/jit/morphblock.cpp | 128 +++++++------- src/coreclr/jit/optimizer.cpp | 1 + src/coreclr/jit/rationalize.cpp | 4 - src/coreclr/jit/ssabuilder.cpp | 2 +- src/coreclr/jit/valuenum.cpp | 38 ++-- .../JIT/Methodical/xxblk/dynblk_order.il | 114 ++++++++++++ .../Methodical/xxblk/dynblk_order_d.ilproj | 12 ++ .../Methodical/xxblk/dynblk_order_ro.ilproj | 13 ++ .../DynBlkNullAssertions.cs | 42 +++++ .../DynBlkNullAssertions.csproj | 13 ++ 27 files changed, 407 insertions(+), 398 deletions(-) create mode 100644 src/tests/JIT/Methodical/xxblk/dynblk_order.il create mode 100644 src/tests/JIT/Methodical/xxblk/dynblk_order_d.ilproj create mode 100644 src/tests/JIT/Methodical/xxblk/dynblk_order_ro.ilproj create mode 100644 src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.cs create mode 100644 src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.csproj diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 51fad24772b47..4e0e708be326c 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2576,7 +2576,6 @@ void Compiler::optAssertionGen(GenTree* tree) case GT_OBJ: case GT_BLK: - case GT_DYN_BLK: case GT_IND: // R-value indirections create non-null assertions, but not all indirections are R-values. // Those under ADDR nodes or on the LHS of ASGs are "locations", and will not end up @@ -4626,9 +4625,9 @@ GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree, case GT_OBJ: case GT_BLK: - case GT_DYN_BLK: case GT_IND: case GT_NULLCHECK: + case GT_STORE_DYN_BLK: return optAssertionProp_Ind(assertions, tree, stmt); case GT_BOUNDS_CHECK: diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index ff4e0b2d3c285..009c0d9ad41dd 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -1912,7 +1912,7 @@ void CodeGen::genSetBlockSize(GenTreeBlk* blkNode, regNumber sizeReg) } else { - GenTree* sizeNode = blkNode->AsDynBlk()->gtDynamicSize; + GenTree* sizeNode = blkNode->AsStoreDynBlk()->gtDynamicSize; inst_Mov(sizeNode->TypeGet(), sizeReg, sizeNode->GetRegNum(), /* canSkip */ true); } } @@ -2022,7 +2022,7 @@ void CodeGen::genConsumeBlockOp(GenTreeBlk* blkNode, regNumber dstReg, regNumber // in the case where the size is a constant (i.e. it is not GT_STORE_DYN_BLK). if (blkNode->OperGet() == GT_STORE_DYN_BLK) { - genConsumeReg(blkNode->AsDynBlk()->gtDynamicSize); + genConsumeReg(blkNode->AsStoreDynBlk()->gtDynamicSize); } // Next, perform any necessary moves. diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 56701ceb08fad..be0c4393cab8b 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -9389,7 +9389,6 @@ void cTreeFlags(Compiler* comp, GenTree* tree) FALLTHROUGH; case GT_BLK: - case GT_DYN_BLK: case GT_STORE_BLK: case GT_STORE_DYN_BLK: @@ -9742,7 +9741,7 @@ bool Compiler::lvaIsOSRLocal(unsigned varNum) // void Compiler::gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block) { - assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK, GT_DYN_BLK)); + assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK)); tree->ChangeOper(GT_NULLCHECK); tree->ChangeType(TYP_INT); block->bbFlags |= BBF_HAS_NULLCHECK; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index cfee3b00519f5..0d4a9398481fd 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6404,6 +6404,7 @@ class Compiler GenTree* fgMorphGetStructAddr(GenTree** pTree, CORINFO_CLASS_HANDLE clsHnd, bool isRValue = false); GenTree* fgMorphBlockOperand(GenTree* tree, var_types asgType, unsigned blockWidth, bool isBlkReqd); GenTree* fgMorphCopyBlock(GenTree* tree); + GenTree* fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree); GenTree* fgMorphForRegisterFP(GenTree* tree); GenTree* fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac = nullptr); GenTree* fgOptimizeCast(GenTreeCast* cast); @@ -11501,42 +11502,14 @@ class GenTreeVisitor break; } - case GT_DYN_BLK: - { - GenTreeDynBlk* const dynBlock = node->AsDynBlk(); - - GenTree** op1Use = &dynBlock->gtOp1; - GenTree** op2Use = &dynBlock->gtDynamicSize; - - result = WalkTree(op1Use, dynBlock); - if (result == fgWalkResult::WALK_ABORT) - { - return result; - } - result = WalkTree(op2Use, dynBlock); - if (result == fgWalkResult::WALK_ABORT) - { - return result; - } - break; - } - case GT_STORE_DYN_BLK: { - GenTreeDynBlk* const dynBlock = node->AsDynBlk(); + GenTreeStoreDynBlk* const dynBlock = node->AsStoreDynBlk(); GenTree** op1Use = &dynBlock->gtOp1; GenTree** op2Use = &dynBlock->gtOp2; GenTree** op3Use = &dynBlock->gtDynamicSize; - if (TVisitor::UseExecutionOrder) - { - if (dynBlock->IsReverseOp()) - { - std::swap(op1Use, op2Use); - } - } - result = WalkTree(op1Use, dynBlock); if (result == fgWalkResult::WALK_ABORT) { diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 2379c0e0ac6ac..5a7b17f776f2c 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4347,20 +4347,9 @@ void GenTree::VisitOperands(TVisitor visitor) return; } - case GT_DYN_BLK: - { - GenTreeDynBlk* const dynBlock = this->AsDynBlk(); - if (visitor(dynBlock->gtOp1) == VisitResult::Abort) - { - return; - } - visitor(dynBlock->gtDynamicSize); - return; - } - case GT_STORE_DYN_BLK: { - GenTreeDynBlk* const dynBlock = this->AsDynBlk(); + GenTreeStoreDynBlk* const dynBlock = this->AsStoreDynBlk(); if (visitor(dynBlock->gtOp1) == VisitResult::Abort) { return; diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index ba032045e74b7..21f0f583340a8 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -3910,33 +3910,6 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR) return; } - // Special handling for dynamic block ops. - if (tree->OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK)) - { - GenTreeDynBlk* dynBlk = tree->AsDynBlk(); - GenTree* sizeNode = dynBlk->gtDynamicSize; - GenTree* dstAddr = dynBlk->Addr(); - GenTree* src = dynBlk->Data(); - bool isReverse = dynBlk->IsReverseOp(); - - // We either have a DYN_BLK or a STORE_DYN_BLK. If the latter, we have a - // src (the Data to be stored), and isReverse tells us whether to evaluate - // that before dstAddr. - if (isReverse && (src != nullptr)) - { - fgSetTreeSeqHelper(src, isLIR); - } - fgSetTreeSeqHelper(dstAddr, isLIR); - if (!isReverse && (src != nullptr)) - { - fgSetTreeSeqHelper(src, isLIR); - } - fgSetTreeSeqHelper(sizeNode, isLIR); - - fgSetTreeSeqFinish(dynBlk, isLIR); - return; - } - /* Is it a 'simple' unary/binary operator? */ if (kind & GTK_SMPOP) @@ -4108,8 +4081,9 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR) break; case GT_STORE_DYN_BLK: - case GT_DYN_BLK: - noway_assert(!"DYN_BLK nodes should be sequenced as a special case"); + fgSetTreeSeqHelper(tree->AsStoreDynBlk()->Addr(), isLIR); + fgSetTreeSeqHelper(tree->AsStoreDynBlk()->Data(), isLIR); + fgSetTreeSeqHelper(tree->AsStoreDynBlk()->gtDynamicSize, isLIR); break; default: diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index ead4be3295769..678a2a60409db 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -258,7 +258,6 @@ void GenTree::InitNodeSize() GenTree::s_gtNodeSizes[GT_FIELD] = TREE_NODE_SZ_LARGE; GenTree::s_gtNodeSizes[GT_CMPXCHG] = TREE_NODE_SZ_LARGE; GenTree::s_gtNodeSizes[GT_QMARK] = TREE_NODE_SZ_LARGE; - GenTree::s_gtNodeSizes[GT_DYN_BLK] = TREE_NODE_SZ_LARGE; GenTree::s_gtNodeSizes[GT_STORE_DYN_BLK] = TREE_NODE_SZ_LARGE; GenTree::s_gtNodeSizes[GT_INTRINSIC] = TREE_NODE_SZ_LARGE; GenTree::s_gtNodeSizes[GT_ALLOCOBJ] = TREE_NODE_SZ_LARGE; @@ -320,7 +319,7 @@ void GenTree::InitNodeSize() static_assert_no_msg(sizeof(GenTreeAddrMode) <= TREE_NODE_SZ_SMALL); static_assert_no_msg(sizeof(GenTreeObj) <= TREE_NODE_SZ_SMALL); static_assert_no_msg(sizeof(GenTreeBlk) <= TREE_NODE_SZ_SMALL); - static_assert_no_msg(sizeof(GenTreeDynBlk) <= TREE_NODE_SZ_LARGE); // *** large node + static_assert_no_msg(sizeof(GenTreeStoreDynBlk) <= TREE_NODE_SZ_LARGE); // *** large node static_assert_no_msg(sizeof(GenTreeRetExpr) <= TREE_NODE_SZ_LARGE); // *** large node static_assert_no_msg(sizeof(GenTreeILOffset) <= TREE_NODE_SZ_SMALL); static_assert_no_msg(sizeof(GenTreeClsVar) <= TREE_NODE_SZ_SMALL); @@ -1609,10 +1608,9 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK) Compare(op1->AsCmpXchg()->gtOpComparand, op2->AsCmpXchg()->gtOpComparand); case GT_STORE_DYN_BLK: - case GT_DYN_BLK: - return Compare(op1->AsDynBlk()->Addr(), op2->AsDynBlk()->Addr()) && - Compare(op1->AsDynBlk()->Data(), op2->AsDynBlk()->Data()) && - Compare(op1->AsDynBlk()->gtDynamicSize, op2->AsDynBlk()->gtDynamicSize); + return Compare(op1->AsStoreDynBlk()->Addr(), op2->AsStoreDynBlk()->Addr()) && + Compare(op1->AsStoreDynBlk()->Data(), op2->AsStoreDynBlk()->Data()) && + Compare(op1->AsStoreDynBlk()->gtDynamicSize, op2->AsStoreDynBlk()->gtDynamicSize); default: assert(!"unexpected operator"); @@ -2073,11 +2071,9 @@ unsigned Compiler::gtHashValue(GenTree* tree) break; case GT_STORE_DYN_BLK: - hash = genTreeHashAdd(hash, gtHashValue(tree->AsDynBlk()->Data())); - FALLTHROUGH; - case GT_DYN_BLK: - hash = genTreeHashAdd(hash, gtHashValue(tree->AsDynBlk()->Addr())); - hash = genTreeHashAdd(hash, gtHashValue(tree->AsDynBlk()->gtDynamicSize)); + hash = genTreeHashAdd(hash, gtHashValue(tree->AsStoreDynBlk()->Data())); + hash = genTreeHashAdd(hash, gtHashValue(tree->AsStoreDynBlk()->Addr())); + hash = genTreeHashAdd(hash, gtHashValue(tree->AsStoreDynBlk()->gtDynamicSize)); break; default: @@ -4136,7 +4132,6 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_IND: case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: // In an indirection, the destination address is evaluated prior to the source. // If we have any side effects on the target indirection, @@ -4645,23 +4640,19 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) break; case GT_STORE_DYN_BLK: - case GT_DYN_BLK: - level = gtSetEvalOrder(tree->AsDynBlk()->Addr()); - costEx = tree->AsDynBlk()->Addr()->GetCostEx(); - costSz = tree->AsDynBlk()->Addr()->GetCostSz(); + level = gtSetEvalOrder(tree->AsStoreDynBlk()->Addr()); + costEx = tree->AsStoreDynBlk()->Addr()->GetCostEx(); + costSz = tree->AsStoreDynBlk()->Addr()->GetCostSz(); - if (oper == GT_STORE_DYN_BLK) - { - lvl2 = gtSetEvalOrder(tree->AsDynBlk()->Data()); - level = max(level, lvl2); - costEx += tree->AsDynBlk()->Data()->GetCostEx(); - costSz += tree->AsDynBlk()->Data()->GetCostSz(); - } + lvl2 = gtSetEvalOrder(tree->AsStoreDynBlk()->Data()); + level = max(level, lvl2); + costEx += tree->AsStoreDynBlk()->Data()->GetCostEx(); + costSz += tree->AsStoreDynBlk()->Data()->GetCostSz(); - lvl2 = gtSetEvalOrder(tree->AsDynBlk()->gtDynamicSize); + lvl2 = gtSetEvalOrder(tree->AsStoreDynBlk()->gtDynamicSize); level = max(level, lvl2); - costEx += tree->AsDynBlk()->gtDynamicSize->GetCostEx(); - costSz += tree->AsDynBlk()->gtDynamicSize->GetCostSz(); + costEx += tree->AsStoreDynBlk()->gtDynamicSize->GetCostEx(); + costSz += tree->AsStoreDynBlk()->gtDynamicSize->GetCostSz(); break; default: @@ -4965,25 +4956,9 @@ bool GenTree::TryGetUse(GenTree* operand, GenTree*** pUse) return false; } - case GT_DYN_BLK: - { - GenTreeDynBlk* const dynBlock = this->AsDynBlk(); - if (operand == dynBlock->gtOp1) - { - *pUse = &dynBlock->gtOp1; - return true; - } - if (operand == dynBlock->gtDynamicSize) - { - *pUse = &dynBlock->gtDynamicSize; - return true; - } - return false; - } - case GT_STORE_DYN_BLK: { - GenTreeDynBlk* const dynBlock = this->AsDynBlk(); + GenTreeStoreDynBlk* const dynBlock = this->AsStoreDynBlk(); if (operand == dynBlock->gtOp1) { *pUse = &dynBlock->gtOp1; @@ -5184,7 +5159,8 @@ GenTree* GenTree::gtRetExprVal(BasicBlockFlags* pbbFlags /* = nullptr */) bool GenTree::OperRequiresAsgFlag() { - if (OperIs(GT_ASG) || OperIs(GT_XADD, GT_XORR, GT_XAND, GT_XCHG, GT_LOCKADD, GT_CMPXCHG, GT_MEMORYBARRIER)) + if (OperIs(GT_ASG, GT_STORE_DYN_BLK) || + OperIs(GT_XADD, GT_XORR, GT_XAND, GT_XCHG, GT_LOCKADD, GT_CMPXCHG, GT_MEMORYBARRIER)) { return true; } @@ -5266,7 +5242,6 @@ bool GenTree::OperIsImplicitIndir() const case GT_CMPXCHG: case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: case GT_STORE_BLK: case GT_STORE_OBJ: case GT_STORE_DYN_BLK: @@ -5353,9 +5328,9 @@ bool GenTree::OperMayThrow(Compiler* comp) case GT_IND: case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: - case GT_STORE_BLK: case GT_NULLCHECK: + case GT_STORE_BLK: + case GT_STORE_DYN_BLK: return (((this->gtFlags & GTF_IND_NONFAULTING) == 0) && comp->fgAddrCouldBeNull(this->AsIndir()->Addr())); case GT_ARR_LENGTH: @@ -7430,10 +7405,6 @@ GenTree* Compiler::gtCloneExpr( GenTreeBlk(GT_BLK, tree->TypeGet(), tree->AsBlk()->Addr(), tree->AsBlk()->GetLayout()); break; - case GT_DYN_BLK: - copy = new (this, GT_DYN_BLK) GenTreeDynBlk(tree->AsOp()->gtGetOp1(), tree->AsDynBlk()->gtDynamicSize); - break; - case GT_FIELD: copy = new (this, GT_FIELD) GenTreeField(tree->TypeGet(), tree->AsField()->GetFldObj(), tree->AsField()->gtFldHnd, tree->AsField()->gtFldOffset); @@ -7661,10 +7632,10 @@ GenTree* Compiler::gtCloneExpr( break; case GT_STORE_DYN_BLK: - case GT_DYN_BLK: copy = new (this, oper) - GenTreeDynBlk(gtCloneExpr(tree->AsDynBlk()->Addr(), addFlags, deepVarNum, deepVarVal), - gtCloneExpr(tree->AsDynBlk()->gtDynamicSize, addFlags, deepVarNum, deepVarVal)); + GenTreeStoreDynBlk(gtCloneExpr(tree->AsStoreDynBlk()->Addr(), addFlags, deepVarNum, deepVarVal), + gtCloneExpr(tree->AsStoreDynBlk()->Data(), addFlags, deepVarNum, deepVarVal), + gtCloneExpr(tree->AsStoreDynBlk()->gtDynamicSize, addFlags, deepVarNum, deepVarVal)); break; default: @@ -8348,20 +8319,10 @@ GenTreeUseEdgeIterator::GenTreeUseEdgeIterator(GenTree* node) m_advance = &GenTreeUseEdgeIterator::AdvanceArrOffset; return; - case GT_DYN_BLK: - m_edge = &m_node->AsDynBlk()->Addr(); - assert(*m_edge != nullptr); - m_advance = &GenTreeUseEdgeIterator::AdvanceDynBlk; - return; - case GT_STORE_DYN_BLK: - { - GenTreeDynBlk* const dynBlock = m_node->AsDynBlk(); - m_edge = dynBlock->IsReverseOp() ? &dynBlock->gtOp2 : &dynBlock->gtOp1; + m_edge = &m_node->AsStoreDynBlk()->Addr(); assert(*m_edge != nullptr); - m_advance = &GenTreeUseEdgeIterator::AdvanceStoreDynBlk; - } return; case GT_CALL: @@ -8439,28 +8400,16 @@ void GenTreeUseEdgeIterator::AdvanceArrOffset() assert(*m_edge != nullptr); } -//------------------------------------------------------------------------ -// GenTreeUseEdgeIterator::AdvanceDynBlk: produces the next operand of a DynBlk node and advances the state. -// -void GenTreeUseEdgeIterator::AdvanceDynBlk() -{ - GenTreeDynBlk* const dynBlock = m_node->AsDynBlk(); - - m_edge = &dynBlock->gtDynamicSize; - assert(*m_edge != nullptr); - m_advance = &GenTreeUseEdgeIterator::Terminate; -} - //------------------------------------------------------------------------ // GenTreeUseEdgeIterator::AdvanceStoreDynBlk: produces the next operand of a StoreDynBlk node and advances the state. // void GenTreeUseEdgeIterator::AdvanceStoreDynBlk() { - GenTreeDynBlk* const dynBlock = m_node->AsDynBlk(); + GenTreeStoreDynBlk* const dynBlock = m_node->AsStoreDynBlk(); switch (m_state) { case 0: - m_edge = dynBlock->IsReverseOp() ? &dynBlock->gtOp1 : &dynBlock->gtOp2; + m_edge = &dynBlock->Data(); m_state = 1; break; case 1: @@ -9252,7 +9201,6 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_ case GT_LEA: case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: case GT_STORE_BLK: case GT_STORE_OBJ: case GT_STORE_DYN_BLK: @@ -11071,7 +11019,6 @@ void Compiler::gtDispTree(GenTree* tree, break; case GT_STORE_DYN_BLK: - case GT_DYN_BLK: if (tree->OperIsCopyBlkOp()) { printf(" (copy)"); @@ -11084,12 +11031,12 @@ void Compiler::gtDispTree(GenTree* tree, if (!topOnly) { - if (tree->AsDynBlk()->Data() != nullptr) + gtDispChild(tree->AsStoreDynBlk()->Addr(), indentStack, IIArc, nullptr, topOnly); + if (tree->AsStoreDynBlk()->Data() != nullptr) { - gtDispChild(tree->AsDynBlk()->Data(), indentStack, IIArc, nullptr, topOnly); + gtDispChild(tree->AsStoreDynBlk()->Data(), indentStack, IIArc, nullptr, topOnly); } - gtDispChild(tree->AsDynBlk()->Addr(), indentStack, IIArc, nullptr, topOnly); - gtDispChild(tree->AsDynBlk()->gtDynamicSize, indentStack, IIArcBottom, nullptr, topOnly); + gtDispChild(tree->AsStoreDynBlk()->gtDynamicSize, indentStack, IIArcBottom, nullptr, topOnly); } break; @@ -11482,7 +11429,7 @@ void Compiler::gtDispLIRNode(GenTree* node, const char* prefixMsg /* = nullptr * displayOperand(operand, buf, operandArc, indentStack, prefixIndent); } } - else if (node->OperIsDynBlkOp()) + else if (node->OperIs(GT_STORE_DYN_BLK)) { if (operand == node->AsBlk()->Addr()) { @@ -11494,19 +11441,7 @@ void Compiler::gtDispLIRNode(GenTree* node, const char* prefixMsg /* = nullptr * } else { - assert(operand == node->AsDynBlk()->gtDynamicSize); - displayOperand(operand, "size", operandArc, indentStack, prefixIndent); - } - } - else if (node->OperGet() == GT_DYN_BLK) - { - if (operand == node->AsBlk()->Addr()) - { - displayOperand(operand, "lhs", operandArc, indentStack, prefixIndent); - } - else - { - assert(operand == node->AsDynBlk()->gtDynamicSize); + assert(operand == node->AsStoreDynBlk()->gtDynamicSize); displayOperand(operand, "size", operandArc, indentStack, prefixIndent); } } @@ -14539,14 +14474,14 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags) { if (flags & GTF_ASG) { - // TODO-Cleanup: This only checks for GT_ASG but according to OperRequiresAsgFlag there - // are many more opers that are considered to have an assignment side effect: atomic ops + // TODO-Bug: This only checks for GT_ASG/GT_STORE_DYN_BLK but according to OperRequiresAsgFlag + // there are many more opers that are considered to have an assignment side effect: atomic ops // (GT_CMPXCHG & co.), GT_MEMORYBARRIER (not classified as an atomic op) and HW intrinsic // memory stores. Atomic ops have special handling in gtExtractSideEffList but the others // will simply be dropped is they are ever subject to an "extract side effects" operation. // It is possible that the reason no bugs have yet been observed in this area is that the // other nodes are likely to always be tree roots. - if (tree->OperIs(GT_ASG)) + if (tree->OperIs(GT_ASG, GT_STORE_DYN_BLK)) { return true; } @@ -15272,33 +15207,21 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo GenTree* destAddr = blkNode->Addr(); unsigned width = blkNode->Size(); // Do we care about whether this assigns the entire variable? - if (pIsEntire != nullptr && blkNode->OperIs(GT_DYN_BLK)) + if (pIsEntire != nullptr && blkNode->OperIs(GT_STORE_DYN_BLK)) { - GenTree* blockWidth = blkNode->AsDynBlk()->gtDynamicSize; + GenTree* blockWidth = blkNode->AsStoreDynBlk()->gtDynamicSize; if (blockWidth->IsCnsIntOrI()) { - if (blockWidth->IsIconHandle()) - { - // If it's a handle, it must be a class handle. We only create such block operations - // for initialization of struct types, so the type of the argument(s) will match this - // type, by construction, and be "entire". - assert(blockWidth->IsIconHandle(GTF_ICON_CLASS_HDL)); - width = comp->info.compCompHnd->getClassSize( - CORINFO_CLASS_HANDLE(blockWidth->AsIntConCommon()->IconValue())); - } - else + assert(blockWidth->AsIntConCommon()->FitsInI32()); + width = static_cast(blockWidth->AsIntConCommon()->IconValue()); + + if (width == 0) { - ssize_t swidth = blockWidth->AsIntConCommon()->IconValue(); - assert(swidth >= 0); - // cpblk of size zero exists in the wild (in yacc-generated code in SQL) and is valid IL. - if (swidth == 0) - { - return false; - } - width = unsigned(swidth); + return false; } } } + return destAddr->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire); } // Otherwise... diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 992a2cb1c0f38..085bc17a1e510 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -1235,12 +1235,10 @@ struct GenTree bool OperIsBlkOp(); bool OperIsCopyBlkOp(); bool OperIsInitBlkOp(); - bool OperIsDynBlkOp(); static bool OperIsBlk(genTreeOps gtOper) { - return ((gtOper == GT_BLK) || (gtOper == GT_OBJ) || (gtOper == GT_DYN_BLK) || (gtOper == GT_STORE_BLK) || - (gtOper == GT_STORE_OBJ) || (gtOper == GT_STORE_DYN_BLK)); + return (gtOper == GT_BLK) || (gtOper == GT_OBJ) || OperIsStoreBlk(gtOper); } bool OperIsBlk() const @@ -1248,19 +1246,9 @@ struct GenTree return OperIsBlk(OperGet()); } - static bool OperIsDynBlk(genTreeOps gtOper) - { - return ((gtOper == GT_DYN_BLK) || (gtOper == GT_STORE_DYN_BLK)); - } - - bool OperIsDynBlk() const - { - return OperIsDynBlk(OperGet()); - } - static bool OperIsStoreBlk(genTreeOps gtOper) { - return ((gtOper == GT_STORE_BLK) || (gtOper == GT_STORE_OBJ) || (gtOper == GT_STORE_DYN_BLK)); + return StaticOperIs(gtOper, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYN_BLK); } bool OperIsStoreBlk() const @@ -2078,9 +2066,14 @@ struct GenTree SetAllEffectsFlags(source->gtFlags & GTF_ALL_EFFECT); } - void SetAllEffectsFlags(GenTree* source, GenTree* otherSource) + void SetAllEffectsFlags(GenTree* firstSource, GenTree* secondSource) { - SetAllEffectsFlags((source->gtFlags | otherSource->gtFlags) & GTF_ALL_EFFECT); + SetAllEffectsFlags((firstSource->gtFlags | secondSource->gtFlags) & GTF_ALL_EFFECT); + } + + void SetAllEffectsFlags(GenTree* firstSource, GenTree* secondSource, GenTree* thirdSouce) + { + SetAllEffectsFlags((firstSource->gtFlags | secondSource->gtFlags | thirdSouce->gtFlags) & GTF_ALL_EFFECT); } void SetAllEffectsFlags(GenTreeFlags sourceFlags) @@ -2745,7 +2738,6 @@ class GenTreeUseEdgeIterator final void AdvanceCmpXchg(); void AdvanceArrElem(); void AdvanceArrOffset(); - void AdvanceDynBlk(); void AdvanceStoreDynBlk(); void AdvanceFieldList(); void AdvancePhi(); @@ -5985,8 +5977,6 @@ struct GenTreeAddrMode : public GenTreeOp struct GenTreeIndir : public GenTreeOp { // The address for the indirection. - // Since GenTreeDynBlk derives from this, but is an "EXOP" (i.e. it has extra fields), - // we can't access Op1 and Op2 in the normal manner if we may have a DynBlk. GenTree*& Addr() { return gtOp1; @@ -6053,7 +6043,7 @@ struct GenTreeBlk : public GenTreeIndir void SetLayout(ClassLayout* layout) { - assert((layout != nullptr) || OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK)); + assert((layout != nullptr) || OperIs(GT_STORE_DYN_BLK)); m_layout = layout; } @@ -6070,7 +6060,7 @@ struct GenTreeBlk : public GenTreeIndir // The size of the buffer to be copied. unsigned Size() const { - assert((m_layout != nullptr) || OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK)); + assert((m_layout != nullptr) || OperIs(GT_STORE_DYN_BLK)); return (m_layout != nullptr) ? m_layout->GetSize() : 0; } @@ -6108,7 +6098,7 @@ struct GenTreeBlk : public GenTreeIndir #endif { assert(OperIsBlk(oper)); - assert((layout != nullptr) || OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK)); + assert((layout != nullptr) || OperIs(GT_STORE_DYN_BLK)); gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT); } @@ -6121,7 +6111,7 @@ struct GenTreeBlk : public GenTreeIndir #endif { assert(OperIsBlk(oper)); - assert((layout != nullptr) || OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK)); + assert((layout != nullptr) || OperIs(GT_STORE_DYN_BLK)); gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT); gtFlags |= (data->gtFlags & GTF_ALL_EFFECT); } @@ -6170,28 +6160,33 @@ struct GenTreeObj : public GenTreeBlk #endif }; -// gtDynBlk -- 'dynamic block' (GT_DYN_BLK). +// GenTreeStoreDynBlk -- 'dynamic block store' (GT_STORE_DYN_BLK). // -// This node is used for block values that have a dynamic size. -// Note that such a value can never have GC pointers. - -struct GenTreeDynBlk : public GenTreeBlk +// This node is used to represent stores that have a dynamic size - the "cpblk" and "initblk" +// IL instructions are implemented with it. Note that such stores assume the input has no GC +// pointers in it, and as such do not ever use write barriers. +// +// The "Data()" member of this node will either be a "dummy" IND(struct) node, for "cpblk", or +// the zero constant/INIT_VAL for "initblk". +// +struct GenTreeStoreDynBlk : public GenTreeBlk { public: GenTree* gtDynamicSize; - GenTreeDynBlk(GenTree* addr, GenTree* dynamicSize) - : GenTreeBlk(GT_DYN_BLK, TYP_STRUCT, addr, nullptr), gtDynamicSize(dynamicSize) + GenTreeStoreDynBlk(GenTree* dstAddr, GenTree* data, GenTree* dynamicSize) + : GenTreeBlk(GT_STORE_DYN_BLK, TYP_VOID, dstAddr, data, nullptr), gtDynamicSize(dynamicSize) { - // Conservatively the 'addr' could be null or point into the global heap. - gtFlags |= GTF_EXCEPT | GTF_GLOB_REF; + // Conservatively the 'dstAddr' could be null or point into the global heap. + // Likewise, this is a store and so must be marked with the GTF_ASG flag. + gtFlags |= (GTF_ASG | GTF_EXCEPT | GTF_GLOB_REF); gtFlags |= (dynamicSize->gtFlags & GTF_ALL_EFFECT); } #if DEBUGGABLE_GENTREE protected: friend GenTree; - GenTreeDynBlk() : GenTreeBlk() + GenTreeStoreDynBlk() : GenTreeBlk() { } #endif // DEBUGGABLE_GENTREE @@ -7493,19 +7488,6 @@ inline bool GenTree::OperIsBlkOp() return ((gtOper == GT_ASG) && varTypeIsStruct(AsOp()->gtOp1)) || OperIsStoreBlk(); } -inline bool GenTree::OperIsDynBlkOp() -{ - if (gtOper == GT_ASG) - { - return gtGetOp1()->OperGet() == GT_DYN_BLK; - } - else if (gtOper == GT_STORE_DYN_BLK) - { - return true; - } - return false; -} - inline bool GenTree::OperIsInitBlkOp() { if (!OperIsBlkOp()) diff --git a/src/coreclr/jit/gtlist.h b/src/coreclr/jit/gtlist.h index 5a5afe6aad050..9b40861fef0fd 100644 --- a/src/coreclr/jit/gtlist.h +++ b/src/coreclr/jit/gtlist.h @@ -90,8 +90,7 @@ GTNODE(OBJ , GenTreeObj ,0,(GTK_UNOP|GTK_EXOP)) GTNODE(STORE_OBJ , GenTreeObj ,0,(GTK_BINOP|GTK_EXOP|GTK_NOVALUE)) // Object that MAY have gc pointers, and thus includes the relevant gc layout info. GTNODE(BLK , GenTreeBlk ,0,(GTK_UNOP|GTK_EXOP)) // Block/object with no gc pointers, and with a known size (e.g. a struct with no gc fields) GTNODE(STORE_BLK , GenTreeBlk ,0,(GTK_BINOP|GTK_EXOP|GTK_NOVALUE)) // Block/object with no gc pointers, and with a known size (e.g. a struct with no gc fields) -GTNODE(DYN_BLK , GenTreeDynBlk ,0,GTK_SPECIAL) // Dynamically sized block object -GTNODE(STORE_DYN_BLK , GenTreeDynBlk ,0,(GTK_SPECIAL|GTK_NOVALUE)) // Dynamically sized block object +GTNODE(STORE_DYN_BLK , GenTreeStoreDynBlk ,0,(GTK_SPECIAL|GTK_NOVALUE)) // Dynamically sized block store GTNODE(BOX , GenTreeBox ,0,(GTK_UNOP|GTK_EXOP|GTK_NOTLIR)) GTNODE(FIELD , GenTreeField ,0,(GTK_UNOP|GTK_EXOP)) // Member-field diff --git a/src/coreclr/jit/gtstructs.h b/src/coreclr/jit/gtstructs.h index 591753837fe89..6373011779020 100644 --- a/src/coreclr/jit/gtstructs.h +++ b/src/coreclr/jit/gtstructs.h @@ -92,14 +92,14 @@ GTSTRUCT_2(ClsVar , GT_CLS_VAR, GT_CLS_VAR_ADDR) GTSTRUCT_1(ArgPlace , GT_ARGPLACE) GTSTRUCT_1(CmpXchg , GT_CMPXCHG) GTSTRUCT_1(AddrMode , GT_LEA) -GTSTRUCT_N(Blk , GT_BLK, GT_STORE_BLK, GT_OBJ, GT_STORE_OBJ, GT_DYN_BLK, GT_STORE_DYN_BLK) +GTSTRUCT_N(Blk , GT_BLK, GT_STORE_BLK, GT_OBJ, GT_STORE_OBJ, GT_STORE_DYN_BLK) GTSTRUCT_2(Obj , GT_OBJ, GT_STORE_OBJ) -GTSTRUCT_2(DynBlk , GT_DYN_BLK, GT_STORE_DYN_BLK) +GTSTRUCT_1(StoreDynBlk , GT_STORE_DYN_BLK) GTSTRUCT_1(Qmark , GT_QMARK) GTSTRUCT_1(PhiArg , GT_PHI_ARG) GTSTRUCT_1(Phi , GT_PHI) GTSTRUCT_1(StoreInd , GT_STOREIND) -GTSTRUCT_N(Indir , GT_STOREIND, GT_IND, GT_NULLCHECK, GT_BLK, GT_STORE_BLK, GT_OBJ, GT_STORE_OBJ, GT_DYN_BLK, GT_STORE_DYN_BLK) +GTSTRUCT_N(Indir , GT_STOREIND, GT_IND, GT_NULLCHECK, GT_BLK, GT_STORE_BLK, GT_OBJ, GT_STORE_OBJ, GT_STORE_DYN_BLK) #if FEATURE_ARG_SPLIT GTSTRUCT_2_SPECIAL(PutArgStk, GT_PUTARG_STK, GT_PUTARG_SPLIT) GTSTRUCT_1(PutArgSplit , GT_PUTARG_SPLIT) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 08597695cf2d4..648b4a8493d2f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1864,7 +1864,6 @@ GenTree* Compiler::impNormStructVal(GenTree* structVal, case GT_OBJ: case GT_BLK: - case GT_DYN_BLK: case GT_ASG: // These should already have the appropriate type. assert(structVal->gtType == structType); @@ -17070,19 +17069,29 @@ void Compiler::impImportBlockCode(BasicBlock* block) op3 = impPopStack().val; // Size op2 = impPopStack().val; // Value - op1 = impPopStack().val; // Dest + op1 = impPopStack().val; // Dst addr if (op3->IsCnsIntOrI()) { size = (unsigned)op3->AsIntConCommon()->IconValue(); op1 = new (this, GT_BLK) GenTreeBlk(GT_BLK, TYP_STRUCT, op1, typGetBlkLayout(size)); + op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0, false); } else { - op1 = new (this, GT_DYN_BLK) GenTreeDynBlk(op1, op3); + if (!op2->IsIntegralConst(0)) + { + op2 = gtNewOperNode(GT_INIT_VAL, TYP_INT, op2); + } + + op1 = new (this, GT_STORE_DYN_BLK) GenTreeStoreDynBlk(op1, op2, op3); size = 0; + + if ((prefixFlags & PREFIX_VOLATILE) != 0) + { + op1->gtFlags |= GTF_BLK_VOLATILE; + } } - op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0, false); goto SPILL_APPEND; @@ -17093,29 +17102,35 @@ void Compiler::impImportBlockCode(BasicBlock* block) Verify(false, "bad opcode"); } op3 = impPopStack().val; // Size - op2 = impPopStack().val; // Src - op1 = impPopStack().val; // Dest + op2 = impPopStack().val; // Src addr + op1 = impPopStack().val; // Dst addr - if (op3->IsCnsIntOrI()) + if (op2->OperGet() == GT_ADDR) { - size = (unsigned)op3->AsIntConCommon()->IconValue(); - op1 = new (this, GT_BLK) GenTreeBlk(GT_BLK, TYP_STRUCT, op1, typGetBlkLayout(size)); + op2 = op2->AsOp()->gtOp1; } else { - op1 = new (this, GT_DYN_BLK) GenTreeDynBlk(op1, op3); - size = 0; + op2 = gtNewOperNode(GT_IND, TYP_STRUCT, op2); } - if (op2->OperGet() == GT_ADDR) + + if (op3->IsCnsIntOrI()) { - op2 = op2->AsOp()->gtOp1; + size = (unsigned)op3->AsIntConCommon()->IconValue(); + op1 = new (this, GT_BLK) GenTreeBlk(GT_BLK, TYP_STRUCT, op1, typGetBlkLayout(size)); + op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0, true); } else { - op2 = gtNewOperNode(GT_IND, TYP_STRUCT, op2); + op1 = new (this, GT_STORE_DYN_BLK) GenTreeStoreDynBlk(op1, op2, op3); + size = 0; + + if ((prefixFlags & PREFIX_VOLATILE) != 0) + { + op1->gtFlags |= GTF_BLK_VOLATILE; + } } - op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0, true); goto SPILL_APPEND; case CEE_CPOBJ: diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index 84d7965d395bf..66684e38e1a3b 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -567,26 +567,6 @@ class LocalAddressVisitor final : public GenTreeVisitor PopValue(); break; - case GT_DYN_BLK: - assert(TopValue(2).Node() == node); - assert(TopValue(1).Node() == node->AsDynBlk()->Addr()); - assert(TopValue(0).Node() == node->AsDynBlk()->gtDynamicSize); - - // The block size may be the result of an indirection so we need - // to escape the location that may be associated with it. - EscapeValue(TopValue(0), node); - - if (!TopValue(2).Indir(TopValue(1))) - { - // If the address comes from another indirection (e.g. DYN_BLK(IND(...)) - // then we need to escape the location. - EscapeLocation(TopValue(1), node); - } - - PopValue(); - PopValue(); - break; - case GT_RETURN: if (TopValue(0).Node() != node) { @@ -829,14 +809,13 @@ class LocalAddressVisitor final : public GenTreeVisitor // user - the node that uses the indirection // // Notes: - // This returns 0 for indirection of unknown size, typically GT_DYN_BLK. - // GT_IND nodes that have type TYP_STRUCT are expected to only appears - // on the RHS of an assignment, in which case the LHS size will be used instead. - // Otherwise 0 is returned as well. + // This returns 0 for indirection of unknown size. GT_IND nodes that have type + // TYP_STRUCT are expected to only appears on the RHS of an assignment, in which + // case the LHS size will be used instead. Otherwise 0 is returned as well. // unsigned GetIndirSize(GenTree* indir, GenTree* user) { - assert(indir->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_DYN_BLK, GT_FIELD)); + assert(indir->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_FIELD)); if (indir->TypeGet() != TYP_STRUCT) { @@ -883,7 +862,7 @@ class LocalAddressVisitor final : public GenTreeVisitor case GT_OBJ: return indir->AsBlk()->GetLayout()->GetSize(); default: - assert(indir->OperIs(GT_IND, GT_DYN_BLK)); + assert(indir->OperIs(GT_IND)); return 0; } } diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 4f8d9334249cf..d9592683db798 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -2090,7 +2090,6 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: { bool removed = fgTryRemoveNonLocal(node, &blockRange); if (!removed && node->IsUnusedValue()) diff --git a/src/coreclr/jit/lsraarm64.cpp b/src/coreclr/jit/lsraarm64.cpp index 796a9e8126e6f..f18099f4f573e 100644 --- a/src/coreclr/jit/lsraarm64.cpp +++ b/src/coreclr/jit/lsraarm64.cpp @@ -522,7 +522,6 @@ int LinearScan::BuildNode(GenTree* tree) break; case GT_BLK: - case GT_DYN_BLK: // These should all be eliminated prior to Lowering. assert(!"Non-store block node in Lowering"); srcCount = 0; diff --git a/src/coreclr/jit/lsraarmarch.cpp b/src/coreclr/jit/lsraarmarch.cpp index f85cb81d9e8c3..a8db1264b64af 100644 --- a/src/coreclr/jit/lsraarmarch.cpp +++ b/src/coreclr/jit/lsraarmarch.cpp @@ -785,7 +785,7 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) if (blkNode->OperIs(GT_STORE_DYN_BLK)) { useCount++; - BuildUse(blkNode->AsDynBlk()->gtDynamicSize, sizeRegMask); + BuildUse(blkNode->AsStoreDynBlk()->gtDynamicSize, sizeRegMask); } buildInternalRegisterUses(); diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp index ce1de0b6f4fde..e679f00b3bca3 100644 --- a/src/coreclr/jit/lsraxarch.cpp +++ b/src/coreclr/jit/lsraxarch.cpp @@ -496,7 +496,6 @@ int LinearScan::BuildNode(GenTree* tree) case GT_OBJ: #endif case GT_BLK: - case GT_DYN_BLK: // These should all be eliminated prior to Lowering. assert(!"Non-store block node in Lowering"); srcCount = 0; @@ -1438,7 +1437,7 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) if (blkNode->OperIs(GT_STORE_DYN_BLK)) { useCount++; - BuildUse(blkNode->AsDynBlk()->gtDynamicSize, sizeRegMask); + BuildUse(blkNode->AsStoreDynBlk()->gtDynamicSize, sizeRegMask); } #ifdef TARGET_X86 diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 48007f56ca860..e7a3db4e80afd 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11601,7 +11601,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) break; case GT_OBJ: case GT_BLK: - case GT_DYN_BLK: case GT_IND: // A non-null mac here implies this node is part of an address computation (the tree parent is // GT_ADDR). @@ -14971,23 +14970,7 @@ GenTree* Compiler::fgMorphTree(GenTree* tree, MorphAddrContext* mac) break; case GT_STORE_DYN_BLK: - case GT_DYN_BLK: - if (tree->OperGet() == GT_STORE_DYN_BLK) - { - tree->AsDynBlk()->Data() = fgMorphTree(tree->AsDynBlk()->Data()); - } - tree->AsDynBlk()->Addr() = fgMorphTree(tree->AsDynBlk()->Addr()); - tree->AsDynBlk()->gtDynamicSize = fgMorphTree(tree->AsDynBlk()->gtDynamicSize); - - tree->gtFlags &= ~GTF_CALL; - tree->SetIndirExceptionFlags(this); - - if (tree->OperGet() == GT_STORE_DYN_BLK) - { - tree->gtFlags |= tree->AsDynBlk()->Data()->gtFlags & GTF_ALL_EFFECT; - } - tree->gtFlags |= tree->AsDynBlk()->Addr()->gtFlags & GTF_ALL_EFFECT; - tree->gtFlags |= tree->AsDynBlk()->gtDynamicSize->gtFlags & GTF_ALL_EFFECT; + tree = fgMorphStoreDynBlock(tree->AsStoreDynBlk()); break; default: diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 4dd0325c480f6..d725923478433 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -26,7 +26,6 @@ class MorphInitBlockHelper static GenTree* MorphBlock(Compiler* comp, GenTree* tree, bool isDest); static GenTree* MorphCommaBlock(Compiler* comp, GenTreeOp* firstComma); - static GenTreeBlk* MorphDynamicBlock(Compiler* comp, GenTreeDynBlk* dynBlock); protected: Compiler* m_comp; @@ -36,9 +35,7 @@ class MorphInitBlockHelper GenTree* m_dst = nullptr; GenTree* m_src = nullptr; - unsigned m_blockSize = 0; - bool m_blockSizeIsConst = false; - + unsigned m_blockSize = 0; unsigned m_dstLclNum = BAD_VAR_NUM; GenTreeLclVarCommon* m_dstLclNode = nullptr; LclVarDsc* m_dstVarDsc = nullptr; @@ -212,9 +209,8 @@ void MorphInitBlockHelper::PrepareDst() if (m_dst->IsLocal()) { - m_dstLclNode = m_dst->AsLclVarCommon(); - m_dstVarDsc = m_comp->lvaGetDesc(m_dstLclNode); - m_blockSizeIsConst = true; + m_dstLclNode = m_dst->AsLclVarCommon(); + m_dstVarDsc = m_comp->lvaGetDesc(m_dstLclNode); if (m_dst->OperIs(GT_LCL_VAR)) { @@ -248,21 +244,19 @@ void MorphInitBlockHelper::PrepareDst() else { assert(m_dst == m_dst->gtEffectiveVal() && "the commas were skipped in MorphBlock"); - assert(m_dst->OperIs(GT_IND, GT_BLK, GT_OBJ, GT_DYN_BLK)); + assert(m_dst->OperIs(GT_IND, GT_BLK, GT_OBJ)); GenTree* dstAddr = m_dst->AsIndir()->Addr(); if (m_dst->OperGet() == GT_IND) { assert(m_dst->TypeGet() != TYP_STRUCT); - m_blockSize = genTypeSize(m_dst); - m_blockSizeIsConst = true; + m_blockSize = genTypeSize(m_dst); } else { assert(m_dst->OperIsBlk()); - GenTreeBlk* blk = m_dst->AsBlk(); - m_blockSize = blk->Size(); - m_blockSizeIsConst = !blk->OperIs(GT_DYN_BLK); + GenTreeBlk* blk = m_dst->AsBlk(); + m_blockSize = blk->Size(); } noway_assert(dstAddr->TypeIs(TYP_BYREF, TYP_I_IMPL)); @@ -444,22 +438,10 @@ GenTree* MorphInitBlockHelper::MorphBlock(Compiler* comp, GenTree* tree, bool is return tree; } - GenTreeBlk* blkNode = tree->AsBlk(); - if (blkNode->OperIs(GT_DYN_BLK)) - { - blkNode = MorphDynamicBlock(comp, blkNode->AsDynBlk()); - if (blkNode->OperIs(GT_DYN_BLK)) - { - JITDUMP("MorphBlock after:\n"); - DISPTREE(blkNode); - return blkNode; - } - } - - GenTree* blkAddr = blkNode->Addr(); + GenTree* blkAddr = tree->AsBlk()->Addr(); assert(blkAddr != nullptr); assert(blkAddr->TypeIs(TYP_I_IMPL, TYP_BYREF, TYP_REF)); - // GT_ADDR, GT_LCL_VAR/FLD, GT_ADD, GT_COMMA, GT_CALL, GT_CNST_INT, GT_LCL_VAR/FLD_ADDR + // GT_ADDR, GT_LCL_VAR/FLD, GT_ADD, GT_COMMA, GT_CALL, GT_CNS_INT, GT_LCL_VAR/FLD_ADDR JITDUMP("MorphBlock after:\n"); DISPTREE(tree); @@ -539,41 +521,6 @@ GenTree* MorphInitBlockHelper::MorphCommaBlock(Compiler* comp, GenTreeOp* firstC return res; } -//------------------------------------------------------------------------ -// MorphDynamicBlock: tries to transform a dynamic block as a const block. -// -// static -GenTreeBlk* MorphInitBlockHelper::MorphDynamicBlock(Compiler* comp, GenTreeDynBlk* dynBlock) -{ - if (dynBlock->gtDynamicSize->IsCnsIntOrI()) - { - GenTreeIntCon* dynSize = dynBlock->gtDynamicSize->AsIntCon(); - assert(dynSize->FitsInI32()); - unsigned size = static_cast(dynSize->IconValue()); - // A GT_BLK with size of zero is not supported, - // so if we encounter such a thing we just leave it as a GT_DYN_BLK - if (size != 0) - { - dynBlock->gtDynamicSize = nullptr; - GenTreeBlk* blkNode = dynBlock; - blkNode->ChangeOper(GT_BLK); - blkNode->SetLayout(comp->typGetBlkLayout(size)); - JITDUMP("MorphDynamicBlock: DYN_BLK was morphed into BLK:\n"); - return blkNode; - } - else - { - JITDUMP("MorphDynamicBlock: DYN_BLK with zero size can't be morphed:\n"); - return dynBlock; - } - } - else - { - JITDUMP("MorphDynamicBlock: DYN_BLK with non-const size can't be morphed:\n"); - return dynBlock; - } -} - class MorphCopyBlockHelper : public MorphInitBlockHelper { public: @@ -745,7 +692,7 @@ void MorphCopyBlockHelper::MorphStructCases() if (m_dstVarDsc != nullptr) { - if (m_dstVarDsc->lvPromoted && m_blockSizeIsConst) + if (m_dstVarDsc->lvPromoted) { noway_assert(varTypeIsStruct(m_dstVarDsc)); noway_assert(!m_comp->opts.MinOpts()); @@ -765,7 +712,7 @@ void MorphCopyBlockHelper::MorphStructCases() if (m_srcVarDsc != nullptr) { - if (m_srcVarDsc->lvPromoted && m_blockSizeIsConst) + if (m_srcVarDsc->lvPromoted) { noway_assert(varTypeIsStruct(m_srcVarDsc)); noway_assert(!m_comp->opts.MinOpts()); @@ -912,7 +859,7 @@ void MorphCopyBlockHelper::MorphStructCases() // [000085] -A---------- = long // [000083] D------N---- lclVar long V17 tmp9 // - if (m_blockSizeIsConst && (m_dstVarDsc->lvFieldCnt == 1) && (m_srcVarDsc != nullptr) && + if ((m_dstVarDsc->lvFieldCnt == 1) && (m_srcVarDsc != nullptr) && (m_blockSize == genTypeSize(m_srcVarDsc->TypeGet()))) { // Reject the following tree: @@ -950,7 +897,7 @@ void MorphCopyBlockHelper::MorphStructCases() // [000243] -----+------ \--* addr byref // [000242] D----+-N---- \--* lclVar byref V28 tmp19 // - if (m_blockSizeIsConst && (m_srcVarDsc->lvFieldCnt == 1) && (m_dstVarDsc != nullptr) && + if ((m_srcVarDsc->lvFieldCnt == 1) && (m_dstVarDsc != nullptr) && (m_blockSize == genTypeSize(m_dstVarDsc->TypeGet()))) { // Check for type agreement @@ -1259,7 +1206,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() // Is the address of a local? GenTreeLclVarCommon* lclVarTree = nullptr; bool isEntire = false; - bool* pIsEntire = (m_blockSizeIsConst ? &isEntire : nullptr); + bool* pIsEntire = &isEntire; if (dstAddrClone->DefinesLocalAddr(m_comp, m_blockSize, &lclVarTree, pIsEntire)) { lclVarTree->gtFlags |= GTF_VAR_DEF; @@ -1532,3 +1479,50 @@ GenTree* Compiler::fgMorphInitBlock(GenTree* tree) { return MorphInitBlockHelper::MorphInitBlock(this, tree); } + +//------------------------------------------------------------------------ +// fgMorphStoreDynBlock: Morph a dynamic block store (GT_STORE_DYN_BLK). +// +// Performs full (pre-order and post-order) morphing for a STORE_DYN_BLK. +// +// Arguments: +// tree - The GT_STORE_DYN_BLK tree to morph. +// +// Return Value: +// In case the size turns into a constant - the store, transformed +// into an "ordinary" ASG(BLK, Data()) one, and further morphed by +// "fgMorphInitBlock"/"fgMorphCopyBlock". Otherwise, the original +// tree (fully morphed). +// +GenTree* Compiler::fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree) +{ + tree->Addr() = fgMorphTree(tree->Addr()); + tree->Data() = fgMorphTree(tree->Data()); + tree->gtDynamicSize = fgMorphTree(tree->gtDynamicSize); + + if (tree->gtDynamicSize->IsIntegralConst()) + { + int64_t size = tree->gtDynamicSize->AsIntConCommon()->IntegralValue(); + assert(FitsIn(size)); + + if (size != 0) + { + GenTree* lhs = gtNewBlockVal(tree->Addr(), static_cast(size)); + lhs->SetIndirExceptionFlags(this); + + GenTree* asg = gtNewAssignNode(lhs, tree->Data()); + asg->gtFlags |= (tree->gtFlags & (GTF_ALL_EFFECT | GTF_BLK_VOLATILE | GTF_BLK_UNALIGNED)); + INDEBUG(asg->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + + JITDUMP("MorphStoreDynBlock: trasformed STORE_DYN_BLK into ASG(BLK, Data())\n"); + + return tree->OperIsCopyBlkOp() ? fgMorphCopyBlock(asg) : fgMorphInitBlock(asg); + } + } + + tree->SetAllEffectsFlags(tree->Addr(), tree->Data(), tree->gtDynamicSize); + tree->SetIndirExceptionFlags(this); + tree->gtFlags |= GTF_ASG; + + return tree; +} diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index a6c9ea4d85c68..d633a6d84aecd 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -7965,6 +7965,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) case GT_XCHG: case GT_CMPXCHG: case GT_MEMORYBARRIER: + case GT_STORE_DYN_BLK: { memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } diff --git a/src/coreclr/jit/rationalize.cpp b/src/coreclr/jit/rationalize.cpp index 18503a4ce3c53..72c1591c6d425 100644 --- a/src/coreclr/jit/rationalize.cpp +++ b/src/coreclr/jit/rationalize.cpp @@ -465,7 +465,6 @@ void Rationalizer::RewriteAssignment(LIR::Use& use) case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: { assert(varTypeIsStruct(location)); GenTreeBlk* storeBlk = location->AsBlk(); @@ -478,9 +477,6 @@ void Rationalizer::RewriteAssignment(LIR::Use& use) case GT_OBJ: storeOper = GT_STORE_OBJ; break; - case GT_DYN_BLK: - storeOper = GT_STORE_DYN_BLK; - break; default: unreached(); } diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index 4646769263d37..48fe4d84d9aec 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -732,7 +732,7 @@ void SsaBuilder::RenameDef(GenTreeOp* asgNode, BasicBlock* block) // This is perhaps temporary -- maybe should be done elsewhere. Label GT_INDs on LHS of assignments, so we // can skip these during (at least) value numbering. GenTree* lhs = asgNode->gtGetOp1()->gtEffectiveVal(/*commaOnly*/ true); - if (lhs->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_DYN_BLK)) + if (lhs->OperIs(GT_IND, GT_OBJ, GT_BLK)) { lhs->gtFlags |= GTF_IND_ASG_LHS; } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 9fc0cefa30089..356124ab00e5c 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6208,7 +6208,7 @@ void ValueNumStore::vnDumpZeroObj(Compiler* comp, VNFuncApp* zeroObj) static UINT8 vnfOpAttribs[VNF_COUNT]; static genTreeOps genTreeOpsIllegalAsVNFunc[] = {GT_IND, // When we do heap memory. GT_NULLCHECK, GT_QMARK, GT_COLON, GT_LOCKADD, GT_XADD, GT_XCHG, - GT_CMPXCHG, GT_LCLHEAP, GT_BOX, GT_XORR, GT_XAND, + GT_CMPXCHG, GT_LCLHEAP, GT_BOX, GT_XORR, GT_XAND, GT_STORE_DYN_BLK, // These need special semantics: GT_COMMA, // == second argument (but with exception(s) from first). @@ -9297,6 +9297,30 @@ void Compiler::fgValueNumberTree(GenTree* tree) break; #endif // FEATURE_HW_INTRINSICS + case GT_STORE_DYN_BLK: + { + // Conservatively, mutate the heaps - we don't analyze these rare stores. + // Likewise, any locals possibly defined by them we mark as address-exposed. + fgMutateGcHeap(tree DEBUGARG("dynamic block store")); + + GenTreeStoreDynBlk* store = tree->AsStoreDynBlk(); + ValueNumPair vnpExcSet = ValueNumStore::VNPForEmptyExcSet(); + + // Propagate the exceptions... + vnpExcSet = vnStore->VNPUnionExcSet(store->Addr()->gtVNPair, vnpExcSet); + vnpExcSet = vnStore->VNPUnionExcSet(store->Data()->gtVNPair, vnpExcSet); + vnpExcSet = vnStore->VNPUnionExcSet(store->gtDynamicSize->gtVNPair, vnpExcSet); + + // This is a store, it produces no value. Thus we use VNPForVoid(). + store->gtVNPair = vnStore->VNPWithExc(vnStore->VNPForVoid(), vnpExcSet); + + // Note that we are only adding the exception for the destination address. + // Currently, "Data()" is an explicit indirection in case this is a "cpblk". + assert(store->Data()->gtEffectiveVal()->OperIsIndir() || store->OperIsInitBlkOp()); + fgValueNumberAddExceptionSetForIndirection(store, store->Addr()); + break; + } + case GT_CMPXCHG: // Specialop { // For CMPXCHG and other intrinsics add an arbitrary side effect on GcHeap/ByrefExposed. @@ -9349,17 +9373,6 @@ void Compiler::fgValueNumberTree(GenTree* tree) } break; - // DYN_BLK is always an L-value. - case GT_DYN_BLK: - assert(!tree->CanCSE()); - tree->gtVNPair = vnStore->VNPForVoid(); - tree->gtVNPair = - vnStore->VNPWithExc(tree->gtVNPair, vnStore->VNPExceptionSet(tree->AsDynBlk()->Addr()->gtVNPair)); - tree->gtVNPair = - vnStore->VNPWithExc(tree->gtVNPair, - vnStore->VNPExceptionSet(tree->AsDynBlk()->gtDynamicSize->gtVNPair)); - break; - // FIELD_LIST is an R-value that we currently don't model. case GT_FIELD_LIST: tree->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, tree->TypeGet())); @@ -10985,7 +10998,6 @@ void Compiler::fgValueNumberAddExceptionSet(GenTree* tree) case GT_IND: case GT_BLK: case GT_OBJ: - case GT_DYN_BLK: case GT_NULLCHECK: fgValueNumberAddExceptionSetForIndirection(tree, tree->AsIndir()->Addr()); break; diff --git a/src/tests/JIT/Methodical/xxblk/dynblk_order.il b/src/tests/JIT/Methodical/xxblk/dynblk_order.il new file mode 100644 index 0000000000000..f4d24714c31ad --- /dev/null +++ b/src/tests/JIT/Methodical/xxblk/dynblk_order.il @@ -0,0 +1,114 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Tests that cpblk/initblk importation sastifies ordering constraints. + +.assembly extern System.Runtime { } +.assembly extern System.Console { } +.assembly extern xunit.core {} + +.assembly DynBlkOrder {} + +.typedef [System.Runtime]System.OverflowException as OverflowException +.typedef [System.Runtime]System.IndexOutOfRangeException as IndexOutOfRangeException + +.class DynBlkOrder extends [System.Runtime]System.Object +{ + .method static int32 Main() + { + .custom instance void [xunit.core]Xunit.FactAttribute::.ctor() = ( + 01 00 00 00 + ) + .entrypoint + .locals (void* dst, void*[] srcArr, int32 srcIdx, int32 size) + + ldloca dst + ldloca srcArr + ldloca srcIdx + ldloca size + call void DynBlkOrder::InitValues(void*&, void*[]&, int32&, int32&) + + .try + { + ldloc dst + ldloc srcArr + ldloc srcIdx + ldelem.i + ldloc size + conv.ovf.u + cpblk + + leave FAIL + } + catch IndexOutOfRangeException + { + leave INITBLK + } + catch OverflowException + { + leave FAIL1 + } + + INITBLK: + .try + { + ldloc dst + ldloc srcArr + ldloc srcIdx + ldelem.i + conv.i4 + ldloc size + conv.ovf.u + initblk + + leave FAIL + } + catch IndexOutOfRangeException + { + leave SUCCESS + } + catch OverflowException + { + leave FAIL2 + } + + SUCCESS: + ldc.i4 100 + ret + + FAIL: + ldc.i4 99 + ret + + FAIL1: + ldc.i4 101 + ret + + FAIL2: + ldc.i4 102 + ret + } + + .method static void InitValues(void*& pSrc, void*[]& pSrcArr, int32& pSrcIdx, int32& pSize) noinlining + { + ldarg pSrc + ldc.i4 0 + conv.u + stind.i + + ldarg pSrcArr + ldc.i4 0 + newarr int32 + stind.ref + + ldarg pSrcIdx + ldc.i4 1 + stind.i4 + + ldarg pSize + ldc.i4 -1 + stind.i4 + + ret + } +} diff --git a/src/tests/JIT/Methodical/xxblk/dynblk_order_d.ilproj b/src/tests/JIT/Methodical/xxblk/dynblk_order_d.ilproj new file mode 100644 index 0000000000000..7a8255ed9d1e6 --- /dev/null +++ b/src/tests/JIT/Methodical/xxblk/dynblk_order_d.ilproj @@ -0,0 +1,12 @@ + + + Exe + 1 + + + Full + + + + + diff --git a/src/tests/JIT/Methodical/xxblk/dynblk_order_ro.ilproj b/src/tests/JIT/Methodical/xxblk/dynblk_order_ro.ilproj new file mode 100644 index 0000000000000..1a281b5960752 --- /dev/null +++ b/src/tests/JIT/Methodical/xxblk/dynblk_order_ro.ilproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + PdbOnly + True + + + + + diff --git a/src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.cs b/src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.cs new file mode 100644 index 0000000000000..22b7c3b94e693 --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.cs @@ -0,0 +1,42 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// "cpblk/initblk" should not generate non-null assertions because the +// indirections they represent may not be realized (in case the "size" +// was zero). + +using System; +using System.Runtime.CompilerServices; + +public class DynBlkNullAssertions +{ + public static int Main() + { + if (!TestCpBlk(ref Unsafe.NullRef(), ref Unsafe.NullRef(), 0)) + { + return 101; + } + if (!TestInitBlk(ref Unsafe.NullRef(), 0, 0)) + { + return 102; + } + + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestCpBlk(ref byte dst, ref byte src, uint size) + { + Unsafe.CopyBlock(ref dst, ref src, size); + + return Unsafe.AreSame(ref dst, ref Unsafe.NullRef()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestInitBlk(ref byte dst, byte value, uint size) + { + Unsafe.InitBlock(ref dst, value, size); + + return Unsafe.AreSame(ref dst, ref Unsafe.NullRef()); + } +} diff --git a/src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.csproj b/src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.csproj new file mode 100644 index 0000000000000..5d8fe22529764 --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/DynBlkNullAssertions.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + None + True + + + + +