Skip to content

Commit

Permalink
Fix unnecessary bounds check with ulong index (dotnet#101352)
Browse files Browse the repository at this point in the history
  • Loading branch information
@EgorBo
EgorBo authored May 1, 2024
1 parent a177fbd commit 4326eb7
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 12 deletions.
60 changes: 48 additions & 12 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
//
Expand All @@ -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<INT32>(funcApp.m_args[1]) - 1;
INT32 validIndex = ConstantValue<INT32>(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<INT32>(funcApp.m_args[0]) - 1;
INT32 validIndex = ConstantValue<INT32>(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;
}
}
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/valuenum.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 4326eb7

Please sign in to comment.