Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unnecessary bounds check with ulong index #101352

Merged
merged 6 commits into from
May 1, 2024
Merged

Fix unnecessary bounds check with ulong index #101352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ea28c5
Fix bound checks for ulong idx
EgorBo Apr 21, 2024
5c3e460
Clean up
EgorBo Apr 21, 2024
704198c
fix
EgorBo Apr 21, 2024
fa58d10
Clean up
EgorBo Apr 21, 2024
de1b3c9
Merge branch 'main' into fix-bound-checks-ulong-idx
EgorBo Apr 22, 2024
e412516
Update valuenum.cpp
EgorBo Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 48 additions & 12 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
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
Open in desktop
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
Loading