diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index fbd0bc96fcedcb..ef619bd418b729 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -781,14 +781,6 @@ void Compiler::optPrintAssertion(const AssertionDsc& curAssertion, AssertionInde printf("VN " FMT_VN "", curAssertion.GetOp1().GetVN()); break; - case O1K_BOUND_OPER_BND: - printf("Oper_Bnd " FMT_VN "", curAssertion.GetOp1().GetVN()); - break; - - case O1K_BOUND_LOOP_BND: - printf("Loop_Bnd " FMT_VN "", curAssertion.GetOp1().GetVN()); - break; - case O1K_EXACT_TYPE: printf("ExactType " FMT_VN "", curAssertion.GetOp1().GetVN()); break; @@ -872,10 +864,6 @@ void Compiler::optPrintAssertion(const AssertionDsc& curAssertion, AssertionInde printf("MT(%s)", eeGetClassName(reinterpret_cast(iconVal))); } } - else if (curAssertion.GetOp1().KindIs(O1K_BOUND_OPER_BND, O1K_BOUND_LOOP_BND)) - { - vnStore->vnDump(this, curAssertion.GetOp2().GetVN()); - } else if (curAssertion.GetOp2().IsNullConstant()) { printf("null"); @@ -909,8 +897,9 @@ void Compiler::optPrintAssertion(const AssertionDsc& curAssertion, AssertionInde IntegralRange::Print(curAssertion.GetOp2().GetIntegralRange()); break; - case O2K_CHECKED_BOUND: - printf("VN " FMT_VN "", curAssertion.GetOp2().GetVN()); + case O2K_CHECKED_BOUND_ADD_CNS: + printf("(Checked_Bnd_BinOp " FMT_VN " + %d)", curAssertion.GetOp2().GetCheckedBound(), + curAssertion.GetOp2().GetCheckedBoundConstant()); break; default: @@ -1458,8 +1447,6 @@ void Compiler::optDebugCheckAssertion(const AssertionDsc& assertion) const case O1K_EXACT_TYPE: case O1K_SUBTYPE: case O1K_VN: - case O1K_BOUND_OPER_BND: - case O1K_BOUND_LOOP_BND: assert(!optLocalAssertionProp); break; default: @@ -1556,6 +1543,12 @@ void Compiler::optCreateComplementaryAssertion(AssertionIndex assertionIndex) AssertionDsc reversed = candidateAssertion.Reverse(); optMapComplementary(optAddAssertion(reversed), assertionIndex); } + else if (candidateAssertion.KindIs(OAK_LT_UN, OAK_LE_UN) && + candidateAssertion.GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS)) + { + // Assertions such as "X > checkedBndVN" aren't very useful. + return; + } else if (AssertionDsc::IsReversible(candidateAssertion.GetKind())) { AssertionDsc reversed = candidateAssertion.Reverse(); @@ -1609,28 +1602,80 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) return NO_ASSERTION_INDEX; } - ValueNum relopVN = vnStore->VNConservativeNormalValue(relop->gtVNPair); + ValueNum relopVN = optConservativeNormalVN(relop); + VNFuncApp relopFuncApp; + if (!vnStore->GetVNFunc(relopVN, &relopFuncApp)) + { + // We're expecting a relop here + return NO_ASSERTION_INDEX; + } - // Cases where op1 holds the lhs of the condition and op2 holds the bound arithmetic. - // Loop condition like: "i < bnd +/-k" - // Assertion: "i < bnd +/- k != 0" - if (vnStore->IsVNCompareCheckedBoundArith(relopVN)) + bool isUnsignedRelop; + if (relopFuncApp.FuncIs(VNF_LE, VNF_LT, VNF_GE, VNF_GT)) { - AssertionDsc dsc = AssertionDsc::CreateCompareCheckedBoundArith(this, relopVN, /*withArith*/ true); - AssertionIndex index = optAddAssertion(dsc); - optCreateComplementaryAssertion(index); - return index; + isUnsignedRelop = false; + } + else if (relopFuncApp.FuncIs(VNF_LE_UN, VNF_LT_UN, VNF_GE_UN, VNF_GT_UN)) + { + isUnsignedRelop = true; + } + else + { + // Not a relop we're interested in. + // Assertions for NE/EQ are handled elsewhere. + return NO_ASSERTION_INDEX; } - // Cases where op1 holds the lhs of the condition op2 holds the bound. - // Loop condition like "i < bnd" - // Assertion: "i < bnd != 0" - if (vnStore->IsVNCompareCheckedBound(relopVN)) + VNFunc relopFunc = relopFuncApp.m_func; + ValueNum op1VN = relopFuncApp.m_args[0]; + ValueNum op2VN = relopFuncApp.m_args[1]; + + if ((genActualType(vnStore->TypeOfVN(op1VN)) != TYP_INT) || (genActualType(vnStore->TypeOfVN(op2VN)) != TYP_INT)) { - AssertionDsc dsc = AssertionDsc::CreateCompareCheckedBoundArith(this, relopVN, /*withArith*/ false); - AssertionIndex index = optAddAssertion(dsc); - optCreateComplementaryAssertion(index); - return index; + // For now, we don't have consumers for assertions derived from non-int32 comparisons + return NO_ASSERTION_INDEX; + } + + // "CheckedBnd X" + if (!isUnsignedRelop && vnStore->IsVNCheckedBound(op1VN)) + { + // Move the checked bound to the right side for simplicity + relopFunc = ValueNumStore::SwapRelop(relopFunc); + AssertionDsc dsc = AssertionDsc::CreateCompareCheckedBound(relopFunc, op2VN, op1VN, 0); + AssertionIndex idx = optAddAssertion(dsc); + optCreateComplementaryAssertion(idx); + return idx; + } + + // "X CheckedBnd" + if (!isUnsignedRelop && vnStore->IsVNCheckedBound(op2VN)) + { + AssertionDsc dsc = AssertionDsc::CreateCompareCheckedBound(relopFunc, op1VN, op2VN, 0); + AssertionIndex idx = optAddAssertion(dsc); + optCreateComplementaryAssertion(idx); + return idx; + } + + // "(CheckedBnd + CNS) X" + ValueNum checkedBnd; + int checkedBndCns; + if (!isUnsignedRelop && vnStore->IsVNCheckedBoundAddConst(op1VN, &checkedBnd, &checkedBndCns)) + { + // Move the (CheckedBnd + CNS) part to the right side for simplicity + relopFunc = ValueNumStore::SwapRelop(relopFunc); + AssertionDsc dsc = AssertionDsc::CreateCompareCheckedBound(relopFunc, op2VN, checkedBnd, checkedBndCns); + AssertionIndex idx = optAddAssertion(dsc); + optCreateComplementaryAssertion(idx); + return idx; + } + + // "X (CheckedBnd + CNS)" + if (!isUnsignedRelop && vnStore->IsVNCheckedBoundAddConst(op2VN, &checkedBnd, &checkedBndCns)) + { + AssertionDsc dsc = AssertionDsc::CreateCompareCheckedBound(relopFunc, op1VN, checkedBnd, checkedBndCns); + AssertionIndex idx = optAddAssertion(dsc); + optCreateComplementaryAssertion(idx); + return idx; } // Loop condition like "(uint)i < (uint)bnd" or equivalent @@ -1641,7 +1686,7 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) ValueNum idxVN = vnStore->VNNormalValue(unsignedCompareBnd.vnIdx); ValueNum lenVN = vnStore->VNNormalValue(unsignedCompareBnd.vnBound); - AssertionDsc dsc = AssertionDsc::CreateNoThrowArrBnd(this, idxVN, lenVN); + AssertionDsc dsc = AssertionDsc::CreateNoThrowArrBnd(idxVN, lenVN); AssertionIndex index = optAddAssertion(dsc); if (unsignedCompareBnd.cmpOper == VNF_GE_UN) { @@ -1653,12 +1698,23 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) } // Create "X relop CNS" assertion (both signed and unsigned relops) - if (vnStore->IsVNConstantBound(relopVN) || vnStore->IsVNConstantBoundUnsigned(relopVN)) + // Ignore non-positive constants for unsigned relops as they don't add any useful information. + ssize_t cns; + if (vnStore->IsVNIntegralConstant(op1VN, &cns) && (!isUnsignedRelop || (cns > 0))) { - AssertionDsc dsc = AssertionDsc::CreateConstantLoopBound(this, relopVN); - AssertionIndex index = optAddAssertion(dsc); - optCreateComplementaryAssertion(index); - return index; + relopFunc = ValueNumStore::SwapRelop(relopFunc); + AssertionDsc dsc = AssertionDsc::CreateConstantBound(this, relopFunc, op2VN, op1VN); + AssertionIndex idx = optAddAssertion(dsc); + optCreateComplementaryAssertion(idx); + return idx; + } + + if (vnStore->IsVNIntegralConstant(op2VN, &cns) && (!isUnsignedRelop || (cns > 0))) + { + AssertionDsc dsc = AssertionDsc::CreateConstantBound(this, relopFunc, op1VN, op2VN); + AssertionIndex idx = optAddAssertion(dsc); + optCreateComplementaryAssertion(idx); + return idx; } return NO_ASSERTION_INDEX; @@ -1926,7 +1982,10 @@ void Compiler::optAssertionGen(GenTree* tree) } else { - assertionInfo = optAddAssertion(AssertionDsc::CreateNoThrowArrBnd(this, idxVN, lenVN)); + // GT_BOUNDS_CHECK node provides the following contract: + // * idxVN < lenVN + // * lenVN is non-negative + assertionInfo = optAddAssertion(AssertionDsc::CreateNoThrowArrBnd(idxVN, lenVN)); } } break; @@ -3530,7 +3589,7 @@ void Compiler::optAssertionProp_RangeProperties(ASSERT_VALARG_TP assertions, // array[idx] = 42; // array.Length is known to be non-negative and non-zero here // - if (curAssertion.IsBoundsCheckNoThrow() && (curAssertion.GetOp2().GetVN() == treeVN)) + if (curAssertion.IsBoundsCheckNoThrow() && (curAssertion.GetOp2().GetCheckedBound() == treeVN)) { *isKnownNonNegative = true; *isKnownNonZero = true; @@ -3902,7 +3961,10 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, // Example: currentTree is "X >= Y" and we have an assertion "X >= Y" (or its inverse "X < Y"). // if (curAssertion.IsRelop() && (curAssertion.GetOp1().GetVN() == op1VN) && - (curAssertion.GetOp2().GetVN() == op2VN)) + // O2K_CHECKED_BOUND_ADD_CNS means op2 is decomposed into op2.vn being checked bound + // and op2.cns being the constant offset. We assemble the original relop VN if we want. + // For now, just skip such assertions here. + !curAssertion.GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS) && (curAssertion.GetOp2().GetVN() == op2VN)) { bool isUnsigned; genTreeOps assertionOper = AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned); @@ -4912,8 +4974,12 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree continue; } + assert(curAssertion.GetOp2().GetCheckedBoundConstant() == 0); + assert(curAssertion.GetOp2().IsCheckedBoundNeverNegative()); + // Do we have a previous range check involving the same 'vnLen' upper bound? - if (curAssertion.GetOp2().GetVN() == vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair)) + if (curAssertion.GetOp2().GetCheckedBound() == + vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair)) { // Do we have the exact same lower bound 'vnIdx'? // a[i] followed by a[i] @@ -5227,16 +5293,16 @@ bool Compiler::optCreateJumpTableImpliedAssertions(BasicBlock* switchBb) if (vnStore->IsVNNeverNegative(opVN)) { // Create "X >= value" assertion (both operands are never negative) - ValueNum relop = vnStore->VNForFunc(TYP_INT, VNF_GE, opVN, vnStore->VNForIntCon(value)); - AssertionDsc dsc = AssertionDsc::CreateConstantLoopBound(this, relop); - newAssertIdx = optAddAssertion(dsc); + AssertionDsc dsc = + AssertionDsc::CreateConstantBound(this, VNF_GE, opVN, vnStore->VNForIntCon(value)); + newAssertIdx = optAddAssertion(dsc); } else { // Create "X u>= value" assertion - ValueNum relop = vnStore->VNForFunc(TYP_INT, VNF_GE_UN, opVN, vnStore->VNForIntCon(value)); - AssertionDsc dsc = AssertionDsc::CreateConstantLoopBound(this, relop); - newAssertIdx = optAddAssertion(dsc); + AssertionDsc dsc = + AssertionDsc::CreateConstantBound(this, VNF_GE_UN, opVN, vnStore->VNForIntCon(value)); + newAssertIdx = optAddAssertion(dsc); } } else diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e164ac1b2085c6..0ffca3fd771cd4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7697,8 +7697,6 @@ class Compiler { O1K_LCLVAR, O1K_VN, - O1K_BOUND_OPER_BND, - O1K_BOUND_LOOP_BND, O1K_EXACT_TYPE, O1K_SUBTYPE // NOTE: as of today, only LCLVAR is used by both Local and Global assertion prop @@ -7711,9 +7709,13 @@ class Compiler O2K_LCLVAR_COPY, O2K_CONST_INT, O2K_CONST_DOUBLE, - O2K_CHECKED_BOUND, // Array/Span length (or similar) - O2K_ZEROOBJ, // zeroed struct - O2K_SUBRANGE // IntegralRange + + O2K_CHECKED_BOUND_ADD_CNS, // "checkedBndVN + cns" where op2.vn holds the "checkedBndVN" + // and op2.iconVal holds the "cns". + // "Checked bound" alone doesn't mean anything, + // nor it implies that it's never negative. + O2K_ZEROOBJ, + O2K_SUBRANGE }; struct AssertionDsc @@ -7768,7 +7770,8 @@ class Compiler private: optOp2Kind m_kind; - uint16_t m_encodedIconFlags; // encoded icon gtFlags + bool m_checkedBoundIsNeverNegative; // only meaningful for O2K_CHECKED_BOUND_ADD_CNS kind + uint16_t m_encodedIconFlags; // encoded icon gtFlags ValueNum m_vn; union { @@ -7827,11 +7830,34 @@ class Compiler ValueNum GetVN() const { - assert(KindIs(O2K_CONST_INT, O2K_CONST_DOUBLE, O2K_ZEROOBJ, O2K_CHECKED_BOUND)); + assert(KindIs(O2K_CONST_INT, O2K_CONST_DOUBLE, O2K_ZEROOBJ)); + assert(m_vn != ValueNumStore::NoVN); + return m_vn; + } + + // For "checkedBndVN + cns" form, return the "cns" part. + int GetCheckedBoundConstant() const + { + assert(KindIs(O2K_CHECKED_BOUND_ADD_CNS)); + assert(FitsIn(m_icon.m_iconVal)); + return (int)m_icon.m_iconVal; + } + + // For "checkedBndVN + cns" form, return the "checkedBndVN" part. + // We intentionally don't allow to use it via GetVN() to avoid confusion. + ValueNum GetCheckedBound() const + { + assert(KindIs(O2K_CHECKED_BOUND_ADD_CNS)); assert(m_vn != ValueNumStore::NoVN); return m_vn; } + bool IsCheckedBoundNeverNegative() const + { + assert(KindIs(O2K_CHECKED_BOUND_ADD_CNS)); + return m_checkedBoundIsNeverNegative; + } + optOp2Kind GetKind() const { return m_kind; @@ -7906,15 +7932,6 @@ class Compiler return m_op2; } - bool IsCheckedBoundArithBound() const - { - return (CanPropEqualOrNotEqual() && GetOp1().KindIs(O1K_BOUND_OPER_BND)); - } - bool IsCheckedBoundBound() const - { - return (CanPropEqualOrNotEqual() && GetOp1().KindIs(O1K_BOUND_LOOP_BND)); - } - bool IsCopyAssertion() const { return (KindIs(OAK_EQUAL) && GetOp1().KindIs(O1K_LCLVAR) && GetOp2().KindIs(O2K_LCLVAR_COPY)); @@ -7957,12 +7974,14 @@ class Compiler bool IsBoundsCheckNoThrow() const { - // O1K_VN (idx) u< O2K_VN (len) - return GetOp1().KindIs(O1K_VN) && KindIs(OAK_LT_UN) && GetOp2().KindIs(O2K_CHECKED_BOUND); + // O1K_VN (idx) u< O2K_VN (len) where len is never negative. + // Effectively, it's "idx >= 0 && idx < len" + return GetOp1().KindIs(O1K_VN) && KindIs(OAK_LT_UN) && GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS) && + (GetOp2().GetCheckedBoundConstant() == 0) && GetOp2().IsCheckedBoundNeverNegative(); } // Convert VNFunc to optAssertionKind - static optAssertionKind FromVMFunc(VNFunc func) + static optAssertionKind FromVNFunc(VNFunc func) { switch (func) { @@ -8131,8 +8150,10 @@ class Compiler case O2K_ZEROOBJ: return true; - case O2K_CHECKED_BOUND: - return GetOp2().GetVN() == that.GetOp2().GetVN(); + case O2K_CHECKED_BOUND_ADD_CNS: + return GetOp2().GetCheckedBound() == that.GetOp2().GetCheckedBound() && + GetOp2().GetCheckedBoundConstant() == that.GetOp2().GetCheckedBoundConstant() && + GetOp2().IsCheckedBoundNeverNegative() == that.GetOp2().IsCheckedBoundNeverNegative(); case O2K_LCLVAR_COPY: return GetOp2().GetLclNum() == that.GetOp2().GetLclNum(); @@ -8318,8 +8339,9 @@ class Compiler return dsc; } - // Create a no-throw bounds check assertion: idxVN u< lenVN - static AssertionDsc CreateNoThrowArrBnd(const Compiler* comp, ValueNum idxVN, ValueNum lenVN) + // Create a no-throw bounds check assertion: idxVN u< lenVN where lenVN is never negative + // Effectively, this means "idxVN is in range [0, lenVN)". + static AssertionDsc CreateNoThrowArrBnd(ValueNum idxVN, ValueNum lenVN) { assert(idxVN != ValueNumStore::NoVN); assert(lenVN != ValueNumStore::NoVN); @@ -8328,70 +8350,47 @@ class Compiler dsc.m_assertionKind = OAK_LT_UN; dsc.m_op1.m_kind = O1K_VN; dsc.m_op1.m_vn = idxVN; - dsc.m_op2.m_kind = O2K_CHECKED_BOUND; + dsc.m_op2.m_kind = O2K_CHECKED_BOUND_ADD_CNS; dsc.m_op2.m_vn = lenVN; + + // Normally, "Checked bound" doesn't mean it's never negative, but in this particular case we know it is. + dsc.m_op2.m_checkedBoundIsNeverNegative = true; + dsc.m_op2.m_icon.m_iconVal = 0; return dsc; } - // Create "i < bnd +/- k != 0" or just "i < bnd != 0" assertion - static AssertionDsc CreateCompareCheckedBoundArith(const Compiler* comp, ValueNum relopVN, bool withArith) + // Create "i (bnd + cns)" assertion + static AssertionDsc CreateCompareCheckedBound(VNFunc relop, ValueNum op1VN, ValueNum checkedBndVN, int cns) { - assert(relopVN != ValueNumStore::NoVN); - - if (withArith) - { - assert(comp->vnStore->IsVNCompareCheckedBoundArith(relopVN)); - } - else - { - assert(comp->vnStore->IsVNCompareCheckedBound(relopVN)); - } + assert(op1VN != ValueNumStore::NoVN); + assert(checkedBndVN != ValueNumStore::NoVN); - AssertionDsc dsc = {}; - dsc.m_assertionKind = OAK_NOT_EQUAL; - // TODO-Cleanup: Rename O1K_BOUND_OPER_BND and O1K_BOUND_LOOP_BND to something more meaningful - // Also, consider removing O1K_BOUND_LOOP_BND entirely and use O1K_BOUND_OPER_BND for both cases. - // O1K_BOUND_LOOP_BND is basically "i < bnd +/- 0 != 0". Unless it regresses TP. - dsc.m_op1.m_kind = withArith ? O1K_BOUND_OPER_BND : O1K_BOUND_LOOP_BND; - dsc.m_op1.m_vn = relopVN; - dsc.m_op2.m_kind = O2K_CONST_INT; - dsc.m_op2.m_vn = comp->vnStore->VNZeroForType(TYP_INT); - dsc.m_op2.m_icon.m_iconVal = 0; + AssertionDsc dsc = {}; + dsc.m_assertionKind = FromVNFunc(relop); + dsc.m_op1.m_kind = O1K_VN; + dsc.m_op1.m_vn = op1VN; + dsc.m_op2.m_kind = O2K_CHECKED_BOUND_ADD_CNS; + dsc.m_op2.m_vn = checkedBndVN; + dsc.m_op2.m_icon.m_iconVal = cns; return dsc; } // Create "i < constant" or "i u< constant" assertion - // TODO-Cleanup: Rename it as it's not necessarily a loop bound - static AssertionDsc CreateConstantLoopBound(const Compiler* comp, ValueNum relopVN) + static AssertionDsc CreateConstantBound(const Compiler* comp, VNFunc relop, ValueNum op1VN, ValueNum cnsVN) { - assert(comp->vnStore->IsVNConstantBound(relopVN) || comp->vnStore->IsVNConstantBoundUnsigned(relopVN)); - - VNFuncApp funcApp; - comp->vnStore->GetVNFunc(relopVN, &funcApp); + assert(op1VN != ValueNumStore::NoVN); - VNFunc func = funcApp.m_func; - ValueNum op1 = funcApp.m_args[0]; - ValueNum op2 = funcApp.m_args[1]; - - // if op1 is a constant, then swap the operands and the operator - if (comp->vnStore->IsVNInt32Constant(op1) && !comp->vnStore->IsVNInt32Constant(op2)) - { - std::swap(op1, op2); - func = comp->vnStore->SwapRelop(func); - } - else - { - bool op2IsCns = comp->vnStore->IsVNInt32Constant(op2); - assert(op2IsCns); - } + ssize_t cns; + bool op2IsCns = comp->vnStore->IsVNIntegralConstant(cnsVN, &cns); + assert(op2IsCns); AssertionDsc dsc = {}; - dsc.m_assertionKind = FromVMFunc(func); + dsc.m_assertionKind = FromVNFunc(relop); dsc.m_op1.m_kind = O1K_VN; - dsc.m_op1.m_vn = op1; + dsc.m_op1.m_vn = op1VN; dsc.m_op2.m_kind = O2K_CONST_INT; - dsc.m_op2.m_vn = op2; - dsc.m_op2.m_icon.m_iconVal = comp->vnStore->CoercedConstantValue(op2); + dsc.m_op2.m_vn = cnsVN; + dsc.m_op2.m_icon.m_iconVal = cns; return dsc; } }; diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 1525b2eeae1e19..e3ae60fc3c63ad 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -909,75 +909,44 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, genTreeOps cmpOper = GT_NONE; bool isUnsigned = false; - // Current assertion is of the form (i < len - cns) != 0 - if (canUseCheckedBounds && curAssertion.IsCheckedBoundArithBound()) + // Current assertion is of the form "i (checkedBndVN + cns)" + if (canUseCheckedBounds && + curAssertion.KindIs(Compiler::OAK_GE, Compiler::OAK_GT, Compiler::OAK_LE, Compiler::OAK_LT) && + curAssertion.GetOp2().KindIs(Compiler::O2K_CHECKED_BOUND_ADD_CNS)) { - ValueNumStore::CompareCheckedBoundArithInfo info; - - // Get i, len, cns and < as "info." - comp->vnStore->GetCompareCheckedBoundArithInfo(curAssertion.GetOp1().GetVN(), &info); - - // If we don't have the same variable we are comparing against, bail. - if (normalLclVN != info.cmpOp) - { - continue; - } - - if ((info.arrOper != GT_ADD) && (info.arrOper != GT_SUB)) + if (normalLclVN == curAssertion.GetOp1().GetVN()) { - continue; + cmpOper = Compiler::AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned); + limit = Limit(Limit::keBinOpArray, curAssertion.GetOp2().GetCheckedBound(), + curAssertion.GetOp2().GetCheckedBoundConstant()); } - - // If the operand that operates on the bound is not constant, then done. - if (!comp->vnStore->IsVNInt32Constant(info.arrOp)) + // Check if it's "checkedBndVN1 (checkedBndVN2 + 0)" and normalLclVN is checkedBndVN2 + else if ((normalLclVN == curAssertion.GetOp2().GetCheckedBound()) && + (curAssertion.GetOp2().GetCheckedBoundConstant() == 0)) { - continue; - } - - int cons = comp->vnStore->ConstantValue(info.arrOp); - limit = Limit(Limit::keBinOpArray, info.vnBound, info.arrOper == GT_SUB ? -cons : cons); - cmpOper = (genTreeOps)info.cmpOper; - } - // Current assertion is of the form (i < len) != 0 - else if (canUseCheckedBounds && curAssertion.IsCheckedBoundBound()) - { - ValueNumStore::CompareCheckedBoundArithInfo info; - - // Get the info as "i", "<" and "len" - comp->vnStore->GetCompareCheckedBound(curAssertion.GetOp1().GetVN(), &info); - - // If we don't have the same variable we are comparing against, bail. - if (normalLclVN == info.cmpOp) - { - cmpOper = (genTreeOps)info.cmpOper; - limit = Limit(Limit::keBinOpArray, info.vnBound, 0); - } - else if (normalLclVN == info.vnBound) - { - cmpOper = GenTree::SwapRelop((genTreeOps)info.cmpOper); - limit = Limit(Limit::keBinOpArray, info.cmpOp, 0); + cmpOper = + GenTree::SwapRelop(Compiler::AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned)); + limit = Limit(Limit::keBinOpArray, curAssertion.GetOp1().GetVN(), 0); } else { continue; } + assert(!isUnsigned); } - // Current assertion is of the form (i < 100) != 0 + // Current assertion is of the form "i cns" else if (curAssertion.IsRelop() && curAssertion.GetOp2().KindIs(Compiler::O2K_CONST_INT) && (curAssertion.GetOp1().GetVN() == normalLclVN) && FitsIn(curAssertion.GetOp2().GetIntConstant())) { - isUnsigned = false; - cmpOper = Compiler::AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned); - limit = Limit(Limit::keConstant, static_cast(curAssertion.GetOp2().GetIntConstant())); + cmpOper = Compiler::AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned); + limit = Limit(Limit::keConstant, static_cast(curAssertion.GetOp2().GetIntConstant())); + + // Make sure we don't deal with non-positive constants for unsigned comparisons + assert((curAssertion.GetOp2().GetIntConstant() > 0) || !isUnsigned); } - // Current assertion is of the form i == 100 - else if (curAssertion.IsConstantInt32Assertion()) + // Current assertion is of the form "i == cns" or "i != cns" + else if (curAssertion.IsConstantInt32Assertion() && (curAssertion.GetOp1().GetVN() == normalLclVN)) { - if (curAssertion.GetOp1().GetVN() != normalLclVN) - { - continue; - } - // Ignore GC values/NULL caught by IsConstantInt32Assertion assertion (may happen on 32bit) if (varTypeIsGC(comp->vnStore->TypeOfVN(curAssertion.GetOp2().GetVN()))) { @@ -1030,7 +999,11 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, { // IsBoundsCheckNoThrow is "op1VN (Idx) LT_UN op2VN (Len)" ValueNum indexVN = curAssertion.GetOp1().GetVN(); - ValueNum lenVN = curAssertion.GetOp2().GetVN(); + ValueNum lenVN = curAssertion.GetOp2().GetCheckedBound(); + + assert(curAssertion.GetOp2().GetCheckedBoundConstant() == 0); + assert(curAssertion.GetOp2().IsCheckedBoundNeverNegative()); + if (normalLclVN == indexVN) { if (canUseCheckedBounds) @@ -1085,17 +1058,6 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, } assert(limit.IsBinOpArray() || limit.IsConstant()); - - // TODO-Cleanup: remove this once these assertions are no longer existed. - // All relop-related assertions should be "op1VN relop op2VN" instead of "relopVN ==/!= 0" - bool legacyAssertionKind = - curAssertion.GetOp1().KindIs(Compiler::O1K_BOUND_LOOP_BND, Compiler::O1K_BOUND_OPER_BND); - - // Make sure the assertion is of the form != 0 or == 0 if it isn't a constant assertion. - if (legacyAssertionKind && (curAssertion.GetOp2().GetVN() != comp->vnStore->VNZeroForType(TYP_INT))) - { - continue; - } #ifdef DEBUG if (comp->verbose) { @@ -1124,28 +1086,6 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, arrLenVN = ValueNumStore::NoVN; } - // During assertion prop we add assertions of the form: - // - // (i < length) == 0 - // (i < length) != 0 - // (i < 100) == 0 - // (i < 100) != 0 - // i == 100 - // - // At this point, we have detected that either op1.vn is (i < length) or (i < length + cns) or - // (i < 100) and the op2.vn is 0 or that op1.vn is i and op2.vn is a known constant. - // - // Now, let us check if we are == 0 (i.e., op1 assertion is false) or != 0 (op1 assertion - // is true.). - // - // If we have a non-constant assertion of the form == 0 (i.e., equals false), then reverse relop. - // The relop has to be reversed because we have: (i < length) is false which is the same - // as (i >= length). - if (curAssertion.KindIs(Compiler::OAK_EQUAL) && legacyAssertionKind) - { - cmpOper = GenTree::ReverseRelop(cmpOper); - } - assert(cmpOper != GT_NONE); // Bounds are inclusive, so add -1 for upper bound when "<". But make sure we won't underflow. diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index fcc2db1eb0e495..7ea598add66a78 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2807,6 +2807,13 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V { std::swap(arg0VN, arg1VN); } + + // ... unless one of them is a constant, in which case we want the constant to be arg1 + // NOTE: we don't want to re-order "cns[H] binop cns" + if (IsVNConstantNonHandle(arg0VN)) + { + std::swap(arg0VN, arg1VN); + } } // Have we already assigned a ValueNum for 'func'('arg0VN','arg1VN') ? @@ -7010,46 +7017,6 @@ bool ValueNumStore::IsVNRelop(ValueNum vn) } } -bool ValueNumStore::IsVNConstantBound(ValueNum vn) -{ - VNFuncApp funcApp; - if ((vn != NoVN) && GetVNFunc(vn, &funcApp)) - { - if ((funcApp.m_func == (VNFunc)GT_LE) || (funcApp.m_func == (VNFunc)GT_GE) || - (funcApp.m_func == (VNFunc)GT_LT) || (funcApp.m_func == (VNFunc)GT_GT)) - { - const bool op1IsConst = IsVNInt32Constant(funcApp.m_args[0]); - const bool op2IsConst = IsVNInt32Constant(funcApp.m_args[1]); - - // Technically, we can allow both to be constants, - // but such relops are expected to be constant folded anyway. - return op1IsConst != op2IsConst; - } - } - return false; -} - -bool ValueNumStore::IsVNConstantBoundUnsigned(ValueNum vn) -{ - VNFuncApp funcApp; - if (GetVNFunc(vn, &funcApp)) - { - switch (funcApp.m_func) - { - case VNF_LT_UN: - case VNF_LE_UN: - case VNF_GE_UN: - case VNF_GT_UN: - // Technically, we can allow both to be constants, - // but such relops are expected to be constant folded anyway. - return IsVNPositiveInt32Constant(funcApp.m_args[0]) != IsVNPositiveInt32Constant(funcApp.m_args[1]); - default: - break; - } - } - return false; -} - //------------------------------------------------------------------------ // IsVNPositiveInt32Constant: returns true iff vn is a known Int32 constant that is greater then 0 // @@ -7147,145 +7114,43 @@ bool ValueNumStore::IsVNUnsignedCompareCheckedBound(ValueNum vn, UnsignedCompare return false; } -bool ValueNumStore::IsVNCompareCheckedBound(ValueNum vn) -{ - // Do we have "var < len"? - if (vn == NoVN) - { - return false; - } - - VNFuncApp funcAttr; - if (!GetVNFunc(vn, &funcAttr)) - { - return false; - } - if (funcAttr.m_func != (VNFunc)GT_LE && funcAttr.m_func != (VNFunc)GT_GE && funcAttr.m_func != (VNFunc)GT_LT && - funcAttr.m_func != (VNFunc)GT_GT) - { - return false; - } - if (!IsVNCheckedBound(funcAttr.m_args[0]) && !IsVNCheckedBound(funcAttr.m_args[1])) - { - return false; - } - - return true; -} - -void ValueNumStore::GetCompareCheckedBound(ValueNum vn, CompareCheckedBoundArithInfo* info) -{ - assert(IsVNCompareCheckedBound(vn)); - - // Do we have var < a.len? - VNFuncApp funcAttr; - GetVNFunc(vn, &funcAttr); - - bool isOp1CheckedBound = IsVNCheckedBound(funcAttr.m_args[1]); - if (isOp1CheckedBound) - { - info->cmpOper = funcAttr.m_func; - info->cmpOp = funcAttr.m_args[0]; - info->vnBound = funcAttr.m_args[1]; - } - else - { - info->cmpOper = GenTree::SwapRelop((genTreeOps)funcAttr.m_func); - info->cmpOp = funcAttr.m_args[1]; - info->vnBound = funcAttr.m_args[0]; - } -} - -bool ValueNumStore::IsVNCheckedBoundArith(ValueNum vn) -{ - // Do we have "a.len +or- var" - if (vn == NoVN) - { - return false; - } - - VNFuncApp funcAttr; - - return GetVNFunc(vn, &funcAttr) && // vn is a func. - (funcAttr.m_func == (VNFunc)GT_ADD || funcAttr.m_func == (VNFunc)GT_SUB) && // the func is +/- - (IsVNCheckedBound(funcAttr.m_args[0]) || IsVNCheckedBound(funcAttr.m_args[1])); // either op1 or op2 is a.len -} - -void ValueNumStore::GetCheckedBoundArithInfo(ValueNum vn, CompareCheckedBoundArithInfo* info) -{ - // Do we have a.len +/- var? - assert(IsVNCheckedBoundArith(vn)); - VNFuncApp funcArith; - GetVNFunc(vn, &funcArith); - - bool isOp1CheckedBound = IsVNCheckedBound(funcArith.m_args[1]); - if (isOp1CheckedBound) - { - info->arrOper = funcArith.m_func; - info->arrOp = funcArith.m_args[0]; - info->vnBound = funcArith.m_args[1]; - info->arrOpLHS = true; - } - else - { - info->arrOper = funcArith.m_func; - info->arrOp = funcArith.m_args[1]; - info->vnBound = funcArith.m_args[0]; - info->arrOpLHS = false; - } -} - -bool ValueNumStore::IsVNCompareCheckedBoundArith(ValueNum vn) +//------------------------------------------------------------------------ +// IsVNCheckedBoundAddConst: Checks if the specified vn represents an expression +// of the form "checkedBndVN +/- cns" or its variations. +// +// Arguments: +// vn - Value number to inspect +// checkedBndVN - [Out] checked bound VN in the expression +// addCns - [Out] constant being added to/subtracted from the checked bound VN +// +// Returns: +// true if the specified vn represents an expression of the form "checkedBndVN +/- cns" or its variations, false +// otherwise. +// +bool ValueNumStore::IsVNCheckedBoundAddConst(ValueNum vn, ValueNum* checkedBndVN, int* addCns) { - // Do we have: "var < a.len - var" - if (vn == NoVN) - { - return false; - } - - VNFuncApp funcAttr; - if (!GetVNFunc(vn, &funcAttr)) - { - return false; - } - - // Suitable comparator. - if (funcAttr.m_func != (VNFunc)GT_LE && funcAttr.m_func != (VNFunc)GT_GE && funcAttr.m_func != (VNFunc)GT_LT && - funcAttr.m_func != (VNFunc)GT_GT) - { - return false; - } - - // Either the op0 or op1 is arr len arithmetic. - if (!IsVNCheckedBoundArith(funcAttr.m_args[0]) && !IsVNCheckedBoundArith(funcAttr.m_args[1])) + int cns; + VNFuncApp funcApp; + if (GetVNFunc(vn, &funcApp) && ((funcApp.m_func == VNF_ADD) || (funcApp.m_func == VNF_SUB)) && + IsVNCheckedBound(funcApp.m_args[0]) && IsVNIntegralConstant(funcApp.m_args[1], &cns)) { - return false; - } - - return true; -} - -void ValueNumStore::GetCompareCheckedBoundArithInfo(ValueNum vn, CompareCheckedBoundArithInfo* info) -{ - assert(IsVNCompareCheckedBoundArith(vn)); - - VNFuncApp funcAttr; - GetVNFunc(vn, &funcAttr); + // Normalize "checkedBndVN - cns" into "checkedBndVN + (-cns)" to make it easier for the caller to handle + // both cases. + if (funcApp.m_func == VNF_SUB) + { + if (cns == INT32_MIN) + { + // Avoid possible overflow from negating cns. + return false; + } + cns = -cns; + } - // Check whether op0 or op1 is checked bound arithmetic. - bool isOp1CheckedBoundArith = IsVNCheckedBoundArith(funcAttr.m_args[1]); - if (isOp1CheckedBoundArith) - { - info->cmpOper = funcAttr.m_func; - info->cmpOp = funcAttr.m_args[0]; - GetCheckedBoundArithInfo(funcAttr.m_args[1], info); - } - else - { - info->cmpOper = GenTree::SwapRelop((genTreeOps)funcAttr.m_func); - info->cmpOp = funcAttr.m_args[1]; - GetCheckedBoundArithInfo(funcAttr.m_args[0], info); + *checkedBndVN = funcApp.m_args[0]; + *addCns = cns; + return true; } + return false; } ValueNum ValueNumStore::GetArrForLenVn(ValueNum vn) @@ -10331,18 +10196,6 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) unreached(); } } - else if (IsVNCompareCheckedBound(vn)) - { - CompareCheckedBoundArithInfo info; - GetCompareCheckedBound(vn, &info); - info.dump(this); - } - else if (IsVNCompareCheckedBoundArith(vn)) - { - CompareCheckedBoundArithInfo info; - GetCompareCheckedBoundArithInfo(vn, &info); - info.dump(this); - } else if (IsVNFunc(vn)) { VNFuncApp funcApp; diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index e62122b3cccce1..489c3632287407 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -186,6 +186,17 @@ struct VNFuncApp unsigned m_arity; ValueNum* m_args; + bool FuncIs(VNFunc func) const + { + return m_func == func; + } + + template + bool FuncIs(VNFunc func, T... rest) const + { + return FuncIs(func) || FuncIs(rest...); + } + bool Equals(const VNFuncApp& funcApp) { if (m_func != funcApp.m_func) @@ -1088,42 +1099,6 @@ class ValueNumStore } }; - struct CompareCheckedBoundArithInfo - { - // (vnBound - 1) > vnOp - // (vnBound arrOper arrOp) cmpOper cmpOp - ValueNum vnBound; - unsigned arrOper; - ValueNum arrOp; - bool arrOpLHS; // arrOp is on the left side of cmpOp expression - unsigned cmpOper; - ValueNum cmpOp; - CompareCheckedBoundArithInfo() - : vnBound(NoVN) - , arrOper(GT_NONE) - , arrOp(NoVN) - , arrOpLHS(false) - , cmpOper(GT_NONE) - , cmpOp(NoVN) - { - } -#ifdef DEBUG - void dump(ValueNumStore* vnStore) - { - vnStore->vnDump(vnStore->m_compiler, cmpOp); - printf(" "); - printf(vnStore->VNFuncName((VNFunc)cmpOper)); - printf(" "); - vnStore->vnDump(vnStore->m_compiler, vnBound); - if (arrOper != GT_NONE) - { - printf(vnStore->VNFuncName((VNFunc)arrOper)); - vnStore->vnDump(vnStore->m_compiler, arrOp); - } - } -#endif - }; - // Check if "vn" is "new [] (type handle, size)" bool IsVNNewArr(ValueNum vn, VNFuncApp* funcApp); @@ -1139,32 +1114,12 @@ class ValueNumStore // If "vn" is VN(a.Length) or VN(a.GetLength(n)) then return VN(a); NoVN if VN(a) can't be determined. ValueNum GetArrForLenVn(ValueNum vn); - // Return true with any Relop except for == and != and one operand has to be a 32-bit integer constant. - bool IsVNConstantBound(ValueNum vn); - - // If "vn" is of the form "(uint)var relop cns" for any relop except for == and != - bool IsVNConstantBoundUnsigned(ValueNum vn); - // If "vn" is of the form "(uint)var < (uint)len" (or equivalent) return true. bool IsVNUnsignedCompareCheckedBound(ValueNum vn, UnsignedCompareCheckedBoundInfo* info); - // If "vn" is of the form "var < len" or "len <= var" return true. - bool IsVNCompareCheckedBound(ValueNum vn); - - // If "vn" is checked bound, then populate the "info" fields for the boundVn, cmpOp, cmpOper. - void GetCompareCheckedBound(ValueNum vn, CompareCheckedBoundArithInfo* info); - - // If "vn" is of the form "len +/- var" return true. - bool IsVNCheckedBoundArith(ValueNum vn); - - // If "vn" is checked bound arith, then populate the "info" fields for arrOper, arrVn, arrOp. - void GetCheckedBoundArithInfo(ValueNum vn, CompareCheckedBoundArithInfo* info); - - // If "vn" is of the form "var < len +/- k" return true. - bool IsVNCompareCheckedBoundArith(ValueNum vn); - - // If "vn" is checked bound arith, then populate the "info" fields for cmpOp, cmpOper. - void GetCompareCheckedBoundArithInfo(ValueNum vn, CompareCheckedBoundArithInfo* info); + // If "vn" is of the form "len + cns" return true. + // NOTE: it accepts "cns + len" and "len - cns" as well ("len - cns" is treated as "len + (-cns)"). + bool IsVNCheckedBoundAddConst(ValueNum vn, ValueNum* checkedBndVN, int* addCns); // Returns the flags on the current handle. GTF_ICON_SCOPE_HDL for example. GenTreeFlags GetHandleFlags(ValueNum vn);