From 4326eb7ed4d03f30ce4a4de1eb028ee76fdaaa3c Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 1 May 2024 14:50:21 +0200 Subject: [PATCH] Fix unnecessary bounds check with ulong index (#101352) --- src/coreclr/jit/valuenum.cpp | 60 ++++++++++++++++++++++++++++-------- src/coreclr/jit/valuenum.h | 3 ++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0808d9d971117..909e5ab768af4 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6521,6 +6521,7 @@ bool ValueNumStore::IsVNPositiveInt32Constant(ValueNum vn) // IsVNArrLenUnsignedBound: Checks if the specified vn represents an expression // of one of the following forms: // - "(uint)i < (uint)len" that implies (0 <= i < len) +// - "(ulong)i < (ulong)len" that implies (0 <= i < len) // - "const < (uint)len" that implies "len > const" // - "const <= (uint)len" that implies "len > const - 1" // @@ -6544,49 +6545,57 @@ bool ValueNumStore::IsVNUnsignedCompareCheckedBound(ValueNum vn, UnsignedCompare if ((funcApp.m_func == VNF_LT_UN) || (funcApp.m_func == VNF_GE_UN)) { // We only care about "(uint)i < (uint)len" and its negation "(uint)i >= (uint)len" - if (IsVNCheckedBound(funcApp.m_args[1])) + // Same for (ulong) + ValueNum vnBound = funcApp.m_args[1]; + ValueNum vnIdx = funcApp.m_args[0]; + ValueNum vnCastOp = NoVN; + if (IsVNCheckedBound(vnBound) || (IsVNCastToULong(vnBound, &vnCastOp) && IsVNCheckedBound(vnCastOp))) { - info->vnIdx = funcApp.m_args[0]; + info->vnIdx = vnIdx; info->cmpOper = funcApp.m_func; - info->vnBound = funcApp.m_args[1]; + info->vnBound = (vnCastOp != NoVN) ? vnCastOp : vnBound; return true; } // We care about (uint)len < constant and its negation "(uint)len >= constant" - else if (IsVNPositiveInt32Constant(funcApp.m_args[1]) && IsVNCheckedBound(funcApp.m_args[0])) + else if (IsVNPositiveInt32Constant(vnBound) && IsVNCheckedBound(vnIdx)) { // Change constant < len into (uint)len >= (constant - 1) // to make consuming this simpler (and likewise for it's negation). - INT32 validIndex = ConstantValue(funcApp.m_args[1]) - 1; + INT32 validIndex = ConstantValue(vnBound) - 1; assert(validIndex >= 0); info->vnIdx = VNForIntCon(validIndex); info->cmpOper = (funcApp.m_func == VNF_GE_UN) ? VNF_LT_UN : VNF_GE_UN; - info->vnBound = funcApp.m_args[0]; + info->vnBound = vnIdx; return true; } } else if ((funcApp.m_func == VNF_GT_UN) || (funcApp.m_func == VNF_LE_UN)) { // We only care about "(uint)a.len > (uint)i" and its negation "(uint)a.len <= (uint)i" - if (IsVNCheckedBound(funcApp.m_args[0])) + // Same for (ulong) + ValueNum vnBound = funcApp.m_args[0]; + ValueNum vnIdx = funcApp.m_args[1]; + ValueNum vnCastOp = NoVN; + if (IsVNCheckedBound(vnBound) || (IsVNCastToULong(vnBound, &vnCastOp) && IsVNCheckedBound(vnCastOp))) { - info->vnIdx = funcApp.m_args[1]; + info->vnIdx = vnIdx; // Let's keep a consistent operand order - it's always i < len, never len > i info->cmpOper = (funcApp.m_func == VNF_GT_UN) ? VNF_LT_UN : VNF_GE_UN; - info->vnBound = funcApp.m_args[0]; + info->vnBound = (vnCastOp != NoVN) ? vnCastOp : vnBound; return true; } // Look for constant > (uint)len and its negation "constant <= (uint)len" - else if (IsVNPositiveInt32Constant(funcApp.m_args[0]) && IsVNCheckedBound(funcApp.m_args[1])) + else if (IsVNPositiveInt32Constant(vnBound) && IsVNCheckedBound(vnIdx)) { // Change constant <= (uint)len to (constant - 1) < (uint)len // to make consuming this simpler (and likewise for it's negation). - INT32 validIndex = ConstantValue(funcApp.m_args[0]) - 1; + INT32 validIndex = ConstantValue(vnBound) - 1; assert(validIndex >= 0); info->vnIdx = VNForIntCon(validIndex); info->cmpOper = (funcApp.m_func == VNF_LE_UN) ? VNF_LT_UN : VNF_GE_UN; - info->vnBound = funcApp.m_args[1]; + info->vnBound = vnIdx; return true; } } @@ -6823,6 +6832,33 @@ bool ValueNumStore::IsVNCheckedBound(ValueNum vn) return false; } +//---------------------------------------------------------------------------------- +// IsVNCastToULong: checks whether the given VN represents (ulong)op cast +// +// Arguments: +// vn - VN, presumably, representing (ulong)op cast +// castedOp - [Out] VN of op being cast +// +// Return Value: +// true if the given VN represents (ulong)op cast +// +bool ValueNumStore::IsVNCastToULong(ValueNum vn, ValueNum* castedOp) +{ + VNFuncApp funcApp; + if (GetVNFunc(vn, &funcApp) && (funcApp.m_func == VNF_Cast)) + { + var_types castToType; + bool srcIsUnsigned; + GetCastOperFromVN(funcApp.m_args[1], &castToType, &srcIsUnsigned); + if (srcIsUnsigned && (castToType == TYP_LONG)) + { + *castedOp = funcApp.m_args[0]; + return true; + } + } + return false; +} + void ValueNumStore::SetVNIsCheckedBound(ValueNum vn) { // This is meant to flag VNs for lengths that aren't known at compile time, so we can diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 799d36472b00c..f88de8f55856d 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -905,6 +905,9 @@ class ValueNumStore // of the length argument to a GT_BOUNDS_CHECK node. bool IsVNCheckedBound(ValueNum vn); + // Returns true if the VN is known to be a cast to ulong + bool IsVNCastToULong(ValueNum vn, ValueNum* castedOp); + // Record that a VN is known to appear as the conservative value number of the length // argument to a GT_BOUNDS_CHECK node. void SetVNIsCheckedBound(ValueNum vn);