From b0bf905c32f61c6e9a4eb0d43f061db300263d05 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Tue, 4 Oct 2022 14:31:26 +0100 Subject: [PATCH 1/2] Check for non-valid compare chains (#76566) --- src/coreclr/jit/codegenarm64.cpp | 2 +- src/coreclr/jit/gentree.h | 2 +- src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/lowerarmarch.cpp | 21 ++++++++++----------- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 0cf23388758504..1b31c4fa41717e 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -2696,7 +2696,7 @@ void CodeGen::genCodeForBinary(GenTreeOp* tree) return; } - if (tree->OperIs(GT_AND) && op2->isContainedAndNotIntOrIImmed()) + if (tree->OperIs(GT_AND) && op2->isContainedCompareChainSegment()) { GenCondition cond; bool chain = false; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0b0c2a35456455..caec9335bb43ec 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -911,7 +911,7 @@ struct GenTree } // Node is contained, but it isn't contained due to being a containable int. - bool isContainedAndNotIntOrIImmed() const + bool isContainedCompareChainSegment() const { return isContained() && !isContainedIntOrIImmed(); } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 65ca9b1a2f7fd4..c50b04ea433269 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2753,7 +2753,7 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) #ifdef TARGET_ARM64 // Do not optimise further if op1 has a contained chain. if (op1->OperIs(GT_AND) && - (op1->gtGetOp1()->isContainedAndNotIntOrIImmed() || op1->gtGetOp2()->isContainedAndNotIntOrIImmed())) + (op1->gtGetOp1()->isContainedCompareChainSegment() || op1->gtGetOp2()->isContainedCompareChainSegment())) { return cmp; } diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 36392913cbb2cb..8b68ed0fb503f6 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2255,15 +2255,14 @@ bool Lowering::IsValidCompareChain(GenTree* child, GenTree* parent) { assert(parent->OperIs(GT_AND) || parent->OperIs(GT_SELECT)); - if (child->isContainedAndNotIntOrIImmed()) + if (child->OperIs(GT_AND) || child->OperIsCmpCompare()) { - // Already have a chain. - assert(child->OperIs(GT_AND) || child->OperIsCmpCompare()); - return true; - } - else - { - if (child->OperIs(GT_AND)) + if (child->isContainedCompareChainSegment()) + { + // Already have a chain. + return true; + } + else if (child->OperIs(GT_AND)) { // Count both sides. return IsValidCompareChain(child->AsOp()->gtGetOp2(), child) && @@ -2299,7 +2298,7 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree assert(parent->OperIs(GT_AND) || parent->OperIs(GT_SELECT)); *startOfChain = nullptr; // Nothing found yet. - if (child->isContainedAndNotIntOrIImmed()) + if (child->isContainedCompareChainSegment()) { // Already have a chain. return true; @@ -2310,7 +2309,7 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree if (child->OperIs(GT_AND)) { // If Op2 is not contained, then try to contain it. - if (!child->AsOp()->gtGetOp2()->isContainedAndNotIntOrIImmed()) + if (!child->AsOp()->gtGetOp2()->isContainedCompareChainSegment()) { if (!ContainCheckCompareChain(child->gtGetOp2(), child, startOfChain)) { @@ -2320,7 +2319,7 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree } // If Op1 is not contained, then try to contain it. - if (!child->AsOp()->gtGetOp1()->isContainedAndNotIntOrIImmed()) + if (!child->AsOp()->gtGetOp1()->isContainedCompareChainSegment()) { if (!ContainCheckCompareChain(child->gtGetOp1(), child, startOfChain)) { From 342a7b78e5b3ff5ab3a3328e9b5080c4a8ed77ad Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Tue, 4 Oct 2022 14:31:26 +0100 Subject: [PATCH 2/2] Use parent and child for isContainedCompareChainSegment --- src/coreclr/jit/codegenarm64.cpp | 2 +- src/coreclr/jit/gentree.h | 6 ++--- src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/lowerarmarch.cpp | 40 ++++++++++++++------------------ 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 1b31c4fa41717e..7737953d21240c 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -2696,7 +2696,7 @@ void CodeGen::genCodeForBinary(GenTreeOp* tree) return; } - if (tree->OperIs(GT_AND) && op2->isContainedCompareChainSegment()) + if (tree->isContainedCompareChainSegment(op2)) { GenCondition cond; bool chain = false; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index caec9335bb43ec..6b139b40b07cf9 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -910,10 +910,10 @@ struct GenTree return isContained() && IsCnsIntOrI() && !isUsedFromSpillTemp(); } - // Node is contained, but it isn't contained due to being a containable int. - bool isContainedCompareChainSegment() const + // Node and its child in isolation form a contained compare chain. + bool isContainedCompareChainSegment(GenTree* child) const { - return isContained() && !isContainedIntOrIImmed(); + return (OperIs(GT_AND) && child->isContained() && (child->OperIs(GT_AND) || child->OperIsCmpCompare())); } bool isContainedFltOrDblImmed() const diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index c50b04ea433269..5611761daababc 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2753,7 +2753,7 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) #ifdef TARGET_ARM64 // Do not optimise further if op1 has a contained chain. if (op1->OperIs(GT_AND) && - (op1->gtGetOp1()->isContainedCompareChainSegment() || op1->gtGetOp2()->isContainedCompareChainSegment())) + (op1->isContainedCompareChainSegment(op1->gtGetOp1()) || op1->isContainedCompareChainSegment(op1->gtGetOp2()))) { return cmp; } diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 8b68ed0fb503f6..206ca707942698 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2255,25 +2255,21 @@ bool Lowering::IsValidCompareChain(GenTree* child, GenTree* parent) { assert(parent->OperIs(GT_AND) || parent->OperIs(GT_SELECT)); - if (child->OperIs(GT_AND) || child->OperIsCmpCompare()) + if (parent->isContainedCompareChainSegment(child)) { - if (child->isContainedCompareChainSegment()) - { - // Already have a chain. - return true; - } - else if (child->OperIs(GT_AND)) - { - // Count both sides. - return IsValidCompareChain(child->AsOp()->gtGetOp2(), child) && - IsValidCompareChain(child->AsOp()->gtGetOp1(), child); - } - else if (child->OperIsCmpCompare() && varTypeIsIntegral(child->gtGetOp1()) && - varTypeIsIntegral(child->gtGetOp2())) - { - // Can the child compare be contained. - return IsSafeToContainMem(parent, child); - } + // Already have a chain. + return true; + } + else if (child->OperIs(GT_AND)) + { + // Count both sides. + return IsValidCompareChain(child->AsOp()->gtGetOp2(), child) && + IsValidCompareChain(child->AsOp()->gtGetOp1(), child); + } + else if (child->OperIsCmpCompare() && varTypeIsIntegral(child->gtGetOp1()) && varTypeIsIntegral(child->gtGetOp2())) + { + // Can the child compare be contained. + return IsSafeToContainMem(parent, child); } return false; @@ -2298,9 +2294,9 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree assert(parent->OperIs(GT_AND) || parent->OperIs(GT_SELECT)); *startOfChain = nullptr; // Nothing found yet. - if (child->isContainedCompareChainSegment()) + if (parent->isContainedCompareChainSegment(child)) { - // Already have a chain. + // Already have a contained chain. return true; } // Can the child be contained. @@ -2309,7 +2305,7 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree if (child->OperIs(GT_AND)) { // If Op2 is not contained, then try to contain it. - if (!child->AsOp()->gtGetOp2()->isContainedCompareChainSegment()) + if (!child->isContainedCompareChainSegment(child->AsOp()->gtGetOp2())) { if (!ContainCheckCompareChain(child->gtGetOp2(), child, startOfChain)) { @@ -2319,7 +2315,7 @@ bool Lowering::ContainCheckCompareChain(GenTree* child, GenTree* parent, GenTree } // If Op1 is not contained, then try to contain it. - if (!child->AsOp()->gtGetOp1()->isContainedCompareChainSegment()) + if (!child->isContainedCompareChainSegment(child->AsOp()->gtGetOp1())) { if (!ContainCheckCompareChain(child->gtGetOp1(), child, startOfChain)) {