Skip to content

Remove bounds checks for a[i+c1] followed by a[i+c2] #112532

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

Closed
wants to merge 1 commit into from
Closed

Remove bounds checks for a[i+c1] followed by a[i+c2] #112532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 29 additions & 2 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5448,9 +5448,36 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
}
}
}
else
{
// a[i + cns1] followed by a[i + cns2] where cns2 >= 0 and cns1 >= cns2
VNFuncApp assertedFuncApp;
if (vnStore->GetVNFunc(curAssertion->op1.bnd.vnIdx, &assertedFuncApp) &&
(assertedFuncApp.m_func == VNF_ADD) && vnStore->IsVNInt32Constant(assertedFuncApp.m_args[1]))
{
const int assertedOffset = vnStore->ConstantValue<int>(assertedFuncApp.m_args[1]);

// Now inspect the current index - it has to be either ADD(idx, cns) or just idx
//
int currentOffset = 0;
ValueNum currentIdx = vnCurIdx;
VNFuncApp currentFuncApp;
if (vnStore->GetVNFunc(vnCurIdx, &currentFuncApp) && (currentFuncApp.m_func == VNF_ADD) &&
vnStore->IsVNInt32Constant(currentFuncApp.m_args[1]))
{
currentIdx = currentFuncApp.m_args[0];
currentOffset = vnStore->ConstantValue<int>(currentFuncApp.m_args[1]);
}

if ((currentIdx == assertedFuncApp.m_args[0]) && (currentOffset >= 0) &&
(assertedOffset >= currentOffset))
{
isRedundant = true;
}
Comment on lines +5472 to +5476
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything to worry about with regards to overflow here? E.g. if i + cns1 does not overflow yet i + cns2 does

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, perhaps something like i = -5, cns1 = 5, cns2 = 4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakobbotsch ah, we need to also check the index for being non-negative (via assertions), the diffs are a lot smaller if I check that. I guess #112595 will handle this

}
}
// Extend this to remove additional redundant bounds checks:
// i.e. a[i+1] followed by a[i] by using the VN(i+1) >= VN(i)
// a[i] followed by a[j] when j is known to be >= i
// i.e. a[i] followed by a[j] when j is known to be >= i
// a[i] followed by a[5] when i is known to be >= 5
}

Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2759,10 +2759,10 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V

// We canonicalize commutative operations.
// (Perhaps should eventually handle associative/commutative [AC] ops -- but that gets complicated...)
if (VNFuncIsCommutative(func))
if (VNFuncIsCommutative(func) && !IsVNConstantNonHandle(arg1VN))
{
// Order arg0 arg1 by numerical VN value.
if (arg0VN > arg1VN)
// Order arg0 arg1 by numerical VN value, but keep constant non-handle VNs on the right.
if ((arg0VN > arg1VN) || IsVNConstantNonHandle(arg0VN))
Copy link
Member Author

@EgorBo EgorBo Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous code that was swapping VN operands in a commutative binary func could move constants to the left side which is not desired. Not a correctness issue, but a bit annoying for downstream phases

{
std::swap(arg0VN, arg1VN);
}
Expand Down
Loading