From cf0952b08c11ff560ea5b49f71576251f422e96f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 4 Feb 2023 01:08:14 +0100 Subject: [PATCH 1/5] JIT: Relax reg-optionality validity checks --- src/coreclr/jit/lower.cpp | 137 +++++++++++++++++++-------- src/coreclr/jit/lower.h | 8 +- src/coreclr/jit/lowerarmarch.cpp | 30 +++--- src/coreclr/jit/lowerloongarch64.cpp | 4 +- src/coreclr/jit/lowerxarch.cpp | 76 ++++++--------- 5 files changed, 147 insertions(+), 108 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 79c66577176945..f8285cde29db67 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -78,12 +78,12 @@ void Lowering::MakeSrcRegOptional(GenTree* parentNode, GenTree* childNode) const #ifdef DEBUG // Verify caller of this method checked safety. // - const bool isSafeToContainMem = IsSafeToContainMem(parentNode, childNode); + const bool isSafeToMarkRegOptional = IsSafeToMarkRegOptional(parentNode, childNode); - if (!isSafeToContainMem) + if (!isSafeToMarkRegOptional) { JITDUMP("** Unsafe regOptional of [%06u] in [%06u}\n", comp->dspTreeID(childNode), comp->dspTreeID(parentNode)); - assert(isSafeToContainMem); + assert(isSafeToMarkRegOptional); } #endif } @@ -100,16 +100,11 @@ void Lowering::TryMakeSrcContainedOrRegOptional(GenTree* parentNode, GenTree* ch // HWIntrinsic nodes should use TryGetContainableHWIntrinsicOp and its relevant handling assert(!parentNode->OperIsHWIntrinsic()); - if (!IsSafeToContainMem(parentNode, childNode)) - { - return; - } - - if (IsContainableMemoryOp(childNode)) + if (IsContainableMemoryOp(childNode) && IsSafeToContainMem(parentNode, childNode)) { MakeSrcContained(parentNode, childNode); } - else + else if (IsSafeToMarkRegOptional(parentNode, childNode)) { MakeSrcRegOptional(parentNode, childNode); } @@ -140,32 +135,35 @@ bool Lowering::CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNod } //------------------------------------------------------------------------ -// IsSafeToContainMem: Checks for conflicts between childNode and parentNode, -// and returns 'true' iff memory operand childNode can be contained in parentNode. +// IsInvariantInRange: Check if a node is invariant in the specified range. In +// other words, can 'node' be moved to right before 'endExclusive' without its +// computation changing values? // // Arguments: -// parentNode - any non-leaf node -// childNode - some node that is an input to `parentNode` +// node - The node. +// endExclusive - The exclusive end of the range to check invariance for. // -// Return value: -// true if it is safe to make childNode a contained memory operand. +// Returns: +// True if 'node' can be evaluated at any point between its current +// location and 'parentNode' without giving a different result; otherwise +// false. // -bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const +bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const { // Quick early-out for unary cases // - if (childNode->gtNext == parentNode) + if (node->gtNext == endExclusive) { return true; } m_scratchSideEffects.Clear(); - m_scratchSideEffects.AddNode(comp, childNode); + m_scratchSideEffects.AddNode(comp, node); - for (GenTree* node = childNode->gtNext; node != parentNode; node = node->gtNext) + for (GenTree* cur = node->gtNext; cur != endExclusive; cur = cur->gtNext) { const bool strict = true; - if (m_scratchSideEffects.InterferesWith(comp, node, strict)) + if (m_scratchSideEffects.InterferesWith(comp, cur, strict)) { return false; } @@ -175,31 +173,33 @@ bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const } //------------------------------------------------------------------------ -// IsSafeToContainMem: Checks for conflicts between childNode and grandParentNode -// and returns 'true' iff memory operand childNode can be contained in ancestorNode +// IsInvariantInRange: Check if a node is invariant in the specified range, +// ignoring conflicts with one particular node. // // Arguments: -// grandParentNode - any non-leaf node -// parentNode - parent of `childNode` and an input to `grandParentNode` -// childNode - some node that is an input to `parentNode` +// node - The node. +// endExclusive - The exclusive end of the range to check invariance for. +// ignoreNode - The node to ignore interference checks with, for example +// because it will retain its relative order with 'node'. // -// Return value: -// true if it is safe to make childNode a contained memory operand. +// Returns: +// True if 'node' can be evaluated at any point between its current location +// and 'endExclusive' without giving a different result; otherwise false. // -bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, GenTree* childNode) const +bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive, GenTree* ignoreNode) const { m_scratchSideEffects.Clear(); - m_scratchSideEffects.AddNode(comp, childNode); + m_scratchSideEffects.AddNode(comp, node); - for (GenTree* node = childNode->gtNext; node != grandparentNode; node = node->gtNext) + for (GenTree* cur = node->gtNext; cur != endExclusive; cur = cur->gtNext) { - if (node == parentNode) + if (cur == ignoreNode) { continue; } const bool strict = true; - if (m_scratchSideEffects.InterferesWith(comp, node, strict)) + if (m_scratchSideEffects.InterferesWith(comp, cur, strict)) { return false; } @@ -208,6 +208,58 @@ bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, return true; } +//------------------------------------------------------------------------ +// IsSafeToContainMem: Checks for conflicts between childNode and parentNode, +// and returns 'true' iff memory operand childNode can be contained in parentNode. +// +// Arguments: +// parentNode - any non-leaf node +// childNode - some node that is an input to `parentNode` +// +// Return value: +// true if it is safe to make childNode a contained memory operand. +// +bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const +{ + return IsInvariantInRange(childNode, parentNode); +} + +//------------------------------------------------------------------------ +// IsSafeToContainMem: Checks for conflicts between childNode and grandParentNode +// and returns 'true' iff memory operand childNode can be contained in ancestorNode +// +// Arguments: +// grandParentNode - any non-leaf node +// parentNode - parent of `childNode` and an input to `grandParentNode` +// childNode - some node that is an input to `parentNode` +// +// Return value: +// true if it is safe to make childNode a contained memory operand. +// +bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, GenTree* childNode) const +{ + return IsInvariantInRange(childNode, grandparentNode, parentNode); +} + +bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) const +{ + if (!childNode->OperIs(GT_LCL_VAR)) + { + // LIR edges never interfere. + return true; + } + + LclVarDsc* dsc = comp->lvaGetDesc(childNode->AsLclVarCommon()); + if (!dsc->IsAddressExposed()) + { + // Safe by IR invariants (no assignments occur between parent and node). + return true; + } + + // We expect this to have interference as otherwise we could have marked it contained. + return IsInvariantInRange(childNode, parentNode); +} + //------------------------------------------------------------------------ // LowerNode: this is the main entry point for Lowering. // @@ -2530,15 +2582,16 @@ void Lowering::LowerCFGCall(GenTreeCall* call) } //------------------------------------------------------------------------ -// IsInvariantInRange: Check if a node is invariant in the specified range. In -// other words, can 'node' be moved to right before 'endExclusive' without its -// computation changing values? +// IsCFGCallArgInvariantInRange: A cheap version of IsInvariantInRange to check +// if a node is invariant in the specified range. In other words, can 'node' be +// moved to right before 'endExclusive' without its computation changing +// values? // // Arguments: // node - The node. // endExclusive - The exclusive end of the range to check invariance for. // -bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) +bool Lowering::IsCFGCallArgInvariantInRange(GenTree* node, GenTree* endExclusive) { assert(node->Precedes(endExclusive)); @@ -2603,7 +2656,7 @@ void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node) GenTree* operand = node->AsOp()->gtGetOp1(); JITDUMP("Checking if we can move operand of GT_PUTARG_* node:\n"); DISPTREE(operand); - if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsInvariantInRange(operand, call)) + if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsCFGCallArgInvariantInRange(operand, call)) { JITDUMP("...yes, moving to after validator call\n"); BlockRange().Remove(operand); @@ -5490,7 +5543,7 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par { if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) { - if (IsSafeToContainMem(parent, index)) + if (IsInvariantInRange(index, parent)) { // Check containment safety against the parent node - this will ensure that LEA with the contained // index will itself always be contained. We do not support uncontained LEAs with contained indices. @@ -5513,7 +5566,7 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par if (cast->CastOp()->TypeIs(TYP_INT) && cast->TypeIs(TYP_LONG) && (genTypeSize(targetType) == (1U << shiftBy)) && (scale == 1) && (offset == 0)) { - if (IsSafeToContainMem(parent, index)) + if (IsInvariantInRange(index, parent)) { // Check containment safety against the parent node - this will ensure that LEA with the contained // index will itself always be contained. We do not support uncontained LEAs with contained indices. @@ -7222,7 +7275,7 @@ void Lowering::LowerStoreIndirCommon(GenTreeStoreInd* ind) #if defined(TARGET_ARM64) // Verify containment safety before creating an LEA that must be contained. // - const bool isContainable = IsSafeToContainMem(ind, ind->Addr()); + const bool isContainable = IsInvariantInRange(ind->Addr(), ind); #else const bool isContainable = true; #endif @@ -7283,7 +7336,7 @@ void Lowering::LowerIndir(GenTreeIndir* ind) #if defined(TARGET_ARM64) // Verify containment safety before creating an LEA that must be contained. // - const bool isContainable = (ind->Addr() != nullptr) && IsSafeToContainMem(ind, ind->Addr()); + const bool isContainable = (ind->Addr() != nullptr) && IsInvariantInRange(ind->Addr(), ind); #else const bool isContainable = true; #endif diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 23aef6fbaea2ed..ba44586625bde6 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -123,7 +123,7 @@ class Lowering final : public Phase void LowerBlock(BasicBlock* block); GenTree* LowerNode(GenTree* node); - bool IsInvariantInRange(GenTree* node, GenTree* endExclusive); + bool IsCFGCallArgInvariantInRange(GenTree* node, GenTree* endExclusive); // ------------------------------ // Call Lowering @@ -500,6 +500,9 @@ class Lowering final : public Phase // Checks and makes 'childNode' contained in the 'parentNode' bool CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNode); + bool IsInvariantInRange(GenTree* node, GenTree* endExclusive) const; + bool IsInvariantInRange(GenTree* node, GenTree* endExclusive, GenTree* ignoreNode) const; + // Checks for memory conflicts in the instructions between childNode and parentNode, and returns true if childNode // can be contained. bool IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const; @@ -507,6 +510,9 @@ class Lowering final : public Phase // Similar to above, but allows bypassing a "transparent" parent. bool IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, GenTree* childNode) const; + // Check if marking an operand of a node as reg-optional is safe. + bool IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* node) const; + inline LIR::Range& BlockRange() const { return LIR::AsRange(m_block); diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index a9c981e46d14fe..692eb5b1ecdcf4 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -212,14 +212,14 @@ bool Lowering::IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) co if (parentNode->OperIs(GT_ADD)) { // Find "c + (a * b)" or "(a * b) + c" - return IsSafeToContainMem(parentNode, childNode); + return IsInvariantInRange(childNode, parentNode); } if (parentNode->OperIs(GT_SUB)) { // Find "c - (a * b)" assert(childNode == parentNode->gtGetOp2()); - return IsSafeToContainMem(parentNode, childNode); + return IsInvariantInRange(childNode, parentNode); } return false; @@ -256,7 +256,7 @@ bool Lowering::IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) co { // These operations can still report flags - if (IsSafeToContainMem(parentNode, childNode)) + if (IsInvariantInRange(childNode, parentNode)) { assert(shiftAmountNode->isContained()); return true; @@ -271,7 +271,7 @@ bool Lowering::IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) co if (parentNode->OperIs(GT_CMP, GT_OR, GT_XOR)) { - if (IsSafeToContainMem(parentNode, childNode)) + if (IsInvariantInRange(childNode, parentNode)) { assert(shiftAmountNode->isContained()); return true; @@ -313,7 +313,7 @@ bool Lowering::IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) co { // These operations can still report flags - if (IsSafeToContainMem(parentNode, childNode)) + if (IsInvariantInRange(childNode, parentNode)) { return true; } @@ -327,7 +327,7 @@ bool Lowering::IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) co if (parentNode->OperIs(GT_CMP)) { - if (IsSafeToContainMem(parentNode, childNode)) + if (IsInvariantInRange(childNode, parentNode)) { return true; } @@ -692,7 +692,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT } #endif // !TARGET_ARM - if (!IsSafeToContainMem(blkNode, addrParent, addr)) + if (!IsInvariantInRange(addr, addrParent, blkNode)) { return; } @@ -1966,7 +1966,7 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode) GenTree* addr = indirNode->Addr(); - if ((addr->OperGet() == GT_LEA) && IsSafeToContainMem(indirNode, addr)) + if ((addr->OperGet() == GT_LEA) && IsInvariantInRange(addr, indirNode)) { bool makeContained = true; @@ -2230,13 +2230,13 @@ void Lowering::ContainCheckCast(GenTreeCast* node) srcIsContainable = true; } - if (srcIsContainable && IsSafeToContainMem(node, castOp)) + if (srcIsContainable) { - if (IsContainableMemoryOp(castOp)) + if (IsContainableMemoryOp(castOp) && IsSafeToContainMem(node, castOp)) { MakeSrcContained(node, castOp); } - else + else if (IsSafeToMarkRegOptional(node, castOp)) { castOp->SetRegOptional(); } @@ -2302,7 +2302,7 @@ bool Lowering::IsValidCompareChain(GenTree* child, GenTree* parent) else if (child->OperIsCmpCompare() && varTypeIsIntegral(child->gtGetOp1()) && varTypeIsIntegral(child->gtGetOp2())) { // Can the child compare be contained. - return IsSafeToContainMem(parent, child); + return IsInvariantInRange(child, parent); } return false; @@ -2333,7 +2333,7 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree return true; } // Can the child be contained. - else if (IsSafeToContainMem(parent, child)) + else if (IsInvariantInRange(child, parent)) { if (child->OperIs(GT_AND)) { @@ -2466,7 +2466,7 @@ void Lowering::ContainCheckSelect(GenTreeOp* node) if (cond->OperIsCompare()) { // All compare node types (including TEST_) are containable. - if (IsSafeToContainMem(node, cond)) + if (IsInvariantInRange(cond, node)) { cond->AsOp()->SetContained(); } @@ -2532,7 +2532,7 @@ void Lowering::ContainCheckNeg(GenTreeOp* neg) if ((childNode->gtFlags & GTF_SET_FLAGS)) return; - if (IsSafeToContainMem(neg, childNode)) + if (IsInvariantInRange(childNode, neg)) { MakeSrcContained(neg, childNode); } diff --git a/src/coreclr/jit/lowerloongarch64.cpp b/src/coreclr/jit/lowerloongarch64.cpp index 3cbdf8715b7371..905e555e7935d9 100644 --- a/src/coreclr/jit/lowerloongarch64.cpp +++ b/src/coreclr/jit/lowerloongarch64.cpp @@ -356,7 +356,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT return; } - if (!IsSafeToContainMem(blkNode, addrParent, addr)) + if (!IsInvariantInRange(addr, addrParent, blkNode)) { return; } @@ -629,7 +629,7 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode) #endif // FEATURE_SIMD GenTree* addr = indirNode->Addr(); - if ((addr->OperGet() == GT_LEA) && IsSafeToContainMem(indirNode, addr)) + if ((addr->OperGet() == GT_LEA) && IsInvariantInRange(addr, indirNode)) { MakeSrcContained(indirNode, addr); } diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 5586eb28c9bb66..23372fbeb857c3 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -537,7 +537,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT // Note that the parentNode is always the block node, even if we're dealing with the source address. // The source address is not directly used by the block node but by an IND node and that IND node is // always contained. - if (!IsSafeToContainMem(blkNode, addrParent, addrMode)) + if (!IsInvariantInRange(addrMode, addrParent, blkNode)) { return; } @@ -1300,7 +1300,7 @@ GenTree* Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) break; } - if (!IsSafeToContainMem(node, op1)) + if (!IsInvariantInRange(op1, node)) { // What we're doing here is effectively similar to containment, // except for we're deleting the node entirely, so don't we have @@ -5298,7 +5298,7 @@ void Lowering::ContainCheckIndir(GenTreeIndir* node) MakeSrcContained(node, addr); } } - else if ((addr->OperGet() == GT_LEA) && IsSafeToContainMem(node, addr)) + else if ((addr->OperGet() == GT_LEA) && IsInvariantInRange(addr, node)) { MakeSrcContained(node, addr); } @@ -5329,7 +5329,7 @@ void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) { unsigned swapSize = src->OperIs(GT_BSWAP16) ? 2 : genTypeSize(src); - if ((swapSize == genTypeSize(node)) && IsSafeToContainMem(node, src)) + if ((swapSize == genTypeSize(node)) && IsInvariantInRange(src, node)) { // Prefer containing in the store in case the load has been contained. src->gtGetOp1()->ClearContained(); @@ -5408,7 +5408,7 @@ void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) } } - if (isContainable && IsSafeToContainMem(node, src)) + if (isContainable && IsInvariantInRange(src, node)) { MakeSrcContained(node, src); @@ -5562,30 +5562,27 @@ void Lowering::ContainCheckMul(GenTreeOp* node) } else { - // IsSafeToContainMem is expensive so we call it at most once for each operand - // in this method. If we already called IsSafeToContainMem, it must have returned false; - // otherwise, memOp would be set to the corresponding operand (op1 or op2). if (imm != nullptr) { // Has a contained immediate operand. // Only 'other' operand can be marked as reg optional. assert(other != nullptr); - isSafeToContainOp1 = ((other == op1) && isSafeToContainOp1 && IsSafeToContainMem(node, op1)); - isSafeToContainOp2 = ((other == op2) && isSafeToContainOp2 && IsSafeToContainMem(node, op2)); + isSafeToContainOp1 = ((other == op1) && IsSafeToMarkRegOptional(node, op1)); + isSafeToContainOp2 = ((other == op2) && IsSafeToMarkRegOptional(node, op2)); } else if (hasImpliedFirstOperand) { // Only op2 can be marked as reg optional. isSafeToContainOp1 = false; - isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToMarkRegOptional(node, op2); } else { // If there are no containable operands, we can make either of op1 or op2 // as reg optional. - isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); - isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToMarkRegOptional(node, op1); + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToMarkRegOptional(node, op2); } SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } @@ -5621,11 +5618,11 @@ void Lowering::ContainCheckDivOrMod(GenTreeOp* node) #endif // divisor can be an r/m, but the memory indirection must be of the same size as the divide - if (IsContainableMemoryOp(divisor) && (divisor->TypeGet() == node->TypeGet()) && IsSafeToContainMem(node, divisor)) + if (IsContainableMemoryOp(divisor) && (divisor->TypeGet() == node->TypeGet()) && IsInvariantInRange(divisor, node)) { MakeSrcContained(node, divisor); } - else if (divisorCanBeRegOptional) + else if (divisorCanBeRegOptional && IsSafeToMarkRegOptional(node, divisor)) { // If there are no containable operands, we can make an operand reg optional. // Div instruction allows only divisor to be a memory op. @@ -5833,7 +5830,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) } } - if (!otherOp->isContained() && isSafeToContainOtherOp && IsSafeToContainMem(cmp, otherOp)) + if (!otherOp->isContained() && IsSafeToMarkRegOptional(cmp, otherOp)) { // SSE2 allows only otherOp to be a memory-op. Since otherOp is not // contained, we can mark it reg-optional. @@ -5892,11 +5889,8 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) // if one of them is on stack. GenTree* regOptionalCandidate = op1->IsCnsIntOrI() ? op2 : PreferredRegOptionalOperand(cmp); - // IsSafeToContainMem is expensive so we call it at most once for each operand - // in this method. If we already called IsSafeToContainMem, it must have returned false; - // otherwise, the corresponding operand (op1 or op2) would be contained. - bool setRegOptional = (regOptionalCandidate == op1) ? isSafeToContainOp1 && IsSafeToContainMem(cmp, op1) - : isSafeToContainOp2 && IsSafeToContainMem(cmp, op2); + bool setRegOptional = + (regOptionalCandidate == op1) ? IsSafeToMarkRegOptional(cmp, op1) : IsSafeToMarkRegOptional(cmp, op2); if (setRegOptional) { MakeSrcRegOptional(cmp, regOptionalCandidate); @@ -5932,14 +5926,11 @@ void Lowering::ContainCheckSelect(GenTreeOp* select) if (genTypeSize(op1) == operSize) { - if (IsContainableMemoryOp(op1)) + if (IsContainableMemoryOp(op1) && IsSafeToContainMem(select, op1)) { - if (IsSafeToContainMem(select, op1)) - { - MakeSrcContained(select, op1); - } + MakeSrcContained(select, op1); } - else if (IsSafeToContainMem(select, op1)) + else if (IsSafeToMarkRegOptional(select, op1)) { MakeSrcRegOptional(select, op1); } @@ -5947,14 +5938,11 @@ void Lowering::ContainCheckSelect(GenTreeOp* select) if (genTypeSize(op2) == operSize) { - if (IsContainableMemoryOp(op2)) + if (IsContainableMemoryOp(op2) && IsSafeToContainMem(select, op2)) { - if (IsSafeToContainMem(select, op2)) - { - MakeSrcContained(select, op2); - } + MakeSrcContained(select, op2); } - else if (IsSafeToContainMem(select, op2)) + else if (IsSafeToMarkRegOptional(select, op2)) { MakeSrcRegOptional(select, op2); } @@ -6165,11 +6153,8 @@ void Lowering::ContainCheckBinary(GenTreeOp* node) // Read-Modify-Write (RMW) operation, we can mark its operands // as reg optional. - // IsSafeToContainMem is expensive so we call it at most once for each operand - // in this method. If we already called IsSafeToContainMem, it must have returned false; - // otherwise, directlyEncodable would be true. - isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); - isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + isSafeToContainOp1 = IsSafeToMarkRegOptional(node, op1); + isSafeToContainOp2 = IsSafeToMarkRegOptional(node, op2); SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } @@ -6668,9 +6653,7 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* parentNode, GenTre } } - bool isSafeToContainMem = IsSafeToContainMem(parentNode, childNode); - - *supportsRegOptional = isSafeToContainMem && supportsGeneralLoads; + *supportsRegOptional = supportsGeneralLoads && IsSafeToMarkRegOptional(parentNode, childNode); if (!childNode->OperIsHWIntrinsic()) { @@ -6680,7 +6663,7 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* parentNode, GenTre { if (IsContainableMemoryOp(childNode)) { - canBeContained = isSafeToContainMem; + canBeContained = IsSafeToContainMem(parentNode, childNode); } else if (childNode->IsCnsNonZeroFltOrDbl()) { @@ -6785,7 +6768,7 @@ void Lowering::ContainCheckHWIntrinsicAddr(GenTreeHWIntrinsic* node, GenTree* ad TryCreateAddrMode(addr, true, node); if ((addr->OperIs(GT_CLS_VAR_ADDR, GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR, GT_LEA) || (addr->IsCnsIntOrI() && addr->AsIntConCommon()->FitsInAddrBase(comp))) && - IsSafeToContainMem(node, addr)) + IsInvariantInRange(addr, node)) { MakeSrcContained(node, addr); } @@ -7557,11 +7540,8 @@ void Lowering::ContainCheckFloatBinary(GenTreeOp* node) if (!op1->isContained() && !op2->isContained()) { // If there are no containable operands, we can make an operand reg optional. - // IsSafeToContainMem is expensive so we call it at most once for each operand - // in this method. If we already called IsSafeToContainMem, it must have returned false; - // otherwise, the corresponding operand (op1 or op2) would be contained. - isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); - isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + isSafeToContainOp1 = IsSafeToMarkRegOptional(node, op1); + isSafeToContainOp2 = IsSafeToMarkRegOptional(node, op2); SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } } From 8df13a7f7571d7abccf27607104119cfc30e83f9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 4 Feb 2023 02:04:39 +0100 Subject: [PATCH 2/5] Fix bad refactoring --- src/coreclr/jit/lower.cpp | 4 ++-- src/coreclr/jit/lowerarmarch.cpp | 2 +- src/coreclr/jit/lowerloongarch64.cpp | 2 +- src/coreclr/jit/lowerxarch.cpp | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index f8285cde29db67..d1c57a8d374bd9 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -226,7 +226,7 @@ bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const //------------------------------------------------------------------------ // IsSafeToContainMem: Checks for conflicts between childNode and grandParentNode -// and returns 'true' iff memory operand childNode can be contained in ancestorNode +// and returns 'true' iff memory operand childNode can be contained in grandParentNode. // // Arguments: // grandParentNode - any non-leaf node @@ -257,7 +257,7 @@ bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) } // We expect this to have interference as otherwise we could have marked it contained. - return IsInvariantInRange(childNode, parentNode); + return false; } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 692eb5b1ecdcf4..6946c3d38a2587 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -692,7 +692,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT } #endif // !TARGET_ARM - if (!IsInvariantInRange(addr, addrParent, blkNode)) + if (!IsInvariantInRange(addr, blkNode, addrParent)) { return; } diff --git a/src/coreclr/jit/lowerloongarch64.cpp b/src/coreclr/jit/lowerloongarch64.cpp index 905e555e7935d9..e7dd167deb2fb1 100644 --- a/src/coreclr/jit/lowerloongarch64.cpp +++ b/src/coreclr/jit/lowerloongarch64.cpp @@ -356,7 +356,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT return; } - if (!IsInvariantInRange(addr, addrParent, blkNode)) + if (!IsInvariantInRange(addr, blkNode, addrParent)) { return; } diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 23372fbeb857c3..166a876e80cf64 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -537,7 +537,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT // Note that the parentNode is always the block node, even if we're dealing with the source address. // The source address is not directly used by the block node but by an IND node and that IND node is // always contained. - if (!IsInvariantInRange(addrMode, addrParent, blkNode)) + if (!IsInvariantInRange(addrMode, blkNode, addrParent)) { return; } From bee4fb7036a8c2a25bd626648e662fd3fb1d87e2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 4 Feb 2023 02:44:09 +0100 Subject: [PATCH 3/5] Add a couple of asserts in IsInvariantInRange --- src/coreclr/jit/lower.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d1c57a8d374bd9..2b11962d522aa9 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -150,6 +150,8 @@ bool Lowering::CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNod // bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const { + assert((node != nullptr) && (endExclusive != nullptr)); + // Quick early-out for unary cases // if (node->gtNext == endExclusive) @@ -162,6 +164,7 @@ bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const for (GenTree* cur = node->gtNext; cur != endExclusive; cur = cur->gtNext) { + assert((cur != nullptr) && "Expected first node to precede end node"); const bool strict = true; if (m_scratchSideEffects.InterferesWith(comp, cur, strict)) { @@ -188,11 +191,14 @@ bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const // bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive, GenTree* ignoreNode) const { + assert((node != nullptr) && (endExclusive != nullptr)); + m_scratchSideEffects.Clear(); m_scratchSideEffects.AddNode(comp, node); for (GenTree* cur = node->gtNext; cur != endExclusive; cur = cur->gtNext) { + assert((cur != nullptr) && "Expected first node to precede end node"); if (cur == ignoreNode) { continue; From dc7f280972c4dd86dd9fdfeba890979926c3e4da Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 4 Feb 2023 13:02:00 +0100 Subject: [PATCH 4/5] Add function header --- src/coreclr/jit/lower.cpp | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 2b11962d522aa9..d2f98c7171d8a8 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -247,6 +247,41 @@ bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, return IsInvariantInRange(childNode, grandparentNode, parentNode); } +//------------------------------------------------------------------------ +// IsSafeToMarkRegOptional: Check whether it is safe to mark 'childNode' as +// reg-optional in 'parentNode'. +// +// Arguments: +// parentNode - parent of 'childNode' +// childNode - some node that is an input to `parentNode` +// +// Return value: +// True if it is safe to mark childNode as reg-optional; otherwise false. +// +// Remarks: +// Unlike containment, reg-optionality can only rarely introduce new +// conflicts, because reg-optionality mostly does not cause the child node +// to be evaluated at a new point in time: +// +// 1. For LIR edges (i.e. anything that isn't GT_LCL_VAR) reg-optionality +// indicates that if the edge was spilled to a temp at its def, the parent +// node can use it directly from its spill location without reloading it +// into a register first. This is always safe as as spill temps cannot +// interfere. +// +// For example, an indirection can be marked reg-optional even if there +// is interference between it and its parent; the indirection will still +// be evaluated at its original position, but if the value is spilled to +// stack, then reg-optionality can allow using the value from the spill +// location directly. +// +// 2. For GT_LCL_VAR reg-optionality indicates that the node can use the +// local directly from its home location. IR invariants guarantee that the +// local is not defined between its LIR location and the parent node (see +// CheckLclVarSemanticsHelper). That means the only case where it could +// interfere is due to it being address exposed. So this is the only unsafe +// case. +// bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) const { if (!childNode->OperIs(GT_LCL_VAR)) @@ -262,7 +297,8 @@ bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) return true; } - // We expect this to have interference as otherwise we could have marked it contained. + // We expect this to have interference as otherwise we could have marked it + // contained instead of reg-optional. return false; } From 7ac5c9c46e991a00fc2e1838479c0c14cc021d30 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Feb 2023 14:40:14 +0100 Subject: [PATCH 5/5] Address feedback --- src/coreclr/jit/lower.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d2f98c7171d8a8..2d0d496ac90839 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -145,7 +145,7 @@ bool Lowering::CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNod // // Returns: // True if 'node' can be evaluated at any point between its current -// location and 'parentNode' without giving a different result; otherwise +// location and 'endExclusive' without giving a different result; otherwise // false. // bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const @@ -182,7 +182,7 @@ bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const // Arguments: // node - The node. // endExclusive - The exclusive end of the range to check invariance for. -// ignoreNode - The node to ignore interference checks with, for example +// ignoreNode - A node to ignore interference checks with, for example // because it will retain its relative order with 'node'. // // Returns: @@ -273,7 +273,8 @@ bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, // is interference between it and its parent; the indirection will still // be evaluated at its original position, but if the value is spilled to // stack, then reg-optionality can allow using the value from the spill -// location directly. +// location directly. Similarly, GT_LCL_FLD nodes are never register +// candidates and can be handled the same way. // // 2. For GT_LCL_VAR reg-optionality indicates that the node can use the // local directly from its home location. IR invariants guarantee that the @@ -286,7 +287,7 @@ bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) { if (!childNode->OperIs(GT_LCL_VAR)) { - // LIR edges never interfere. + // LIR edges never interfere. This includes GT_LCL_FLD, see the remarks above. return true; }