Remove O1K_BOUND_OPER_BND and O1K_BOUND_LOOP_BND#124065
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR refactors the JIT's assertion system to remove two legacy assertion kinds (O1K_BOUND_OPER_BND and O1K_BOUND_LOOP_BND) and replaces them with pure relational operator assertions. This simplifies the assertion representation by eliminating the "relop != 0" pattern in favor of direct relop assertions (e.g., "i < len" instead of "(i < len) != 0").
Changes:
- Removed
O1K_BOUND_OPER_BNDandO1K_BOUND_LOOP_BNDassertion kinds and replaced them with pure relop assertions usingOAK_GE,OAK_GT,OAK_LE,OAK_LT - Added
O2K_CHECKED_BOUND_BINOPto distinguish binary operations on checked bounds from plain checked bounds - Simplified
IsVNCheckedBoundArithto accept output parameters and handle constant extraction and normalization directly - Removed obsolete helper methods (
GetCompareCheckedBound,GetCheckedBoundArithInfo,GetCompareCheckedBoundArithInfo) and theCompareCheckedBoundArithInfostruct
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/valuenum.h | Removed CompareCheckedBoundArithInfo struct and obsolete method declarations; updated IsVNCheckedBoundArith signature to accept output parameters |
| src/coreclr/jit/valuenum.cpp | Refactored IsVNCheckedBoundArith to return bound VN and constant via output parameters; removed helper methods and debug dump code for old assertion format |
| src/coreclr/jit/rangecheck.cpp | Updated range checking logic to work with pure relop assertions; simplified extraction of comparison operators using ToCompareOper |
| src/coreclr/jit/compiler.h | Removed old assertion kinds; added O2K_CHECKED_BOUND_BINOP; refactored CreateCompareCheckedBoundArith to create pure relop assertions; updated helper methods |
| src/coreclr/jit/assertionprop.cpp | Removed debug printing code for old assertion kinds; added printing support for O2K_CHECKED_BOUND_BINOP; updated assertion validation |
|
PTAL @AndyAyersMS @dotnet/jit-contrib |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM.
Maybe run fuzzlyn here too?
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@AndyAyersMS I've addressed your feedback, fuzzlyn failures seem unrelated, in fact, most of them should be fixed by #124128 |
2 participants
This PR removes the remaining
O1K_BOUND_OPER_BNDandO1K_BOUND_LOOP_BND.All bounds-related assertions now look like:
op1.vn- just some variableop2.vn- the checked boundop2.icon.iconValue- the int constant added to the checked boundHence, it's unified for e.g. "x < len" and "x < (len - 10)". The "x < len" is effectively "x < (len + 0)".
Overall, it simplifies a lot the search for relevant assertions as we no longer need to decompose relopVNs (and its operands) into pieces every time.
Did a small cleanup along the way. It brings some improvements mostly due to "don't create useless complimentary assertions such as X > BND" check. Also, TP improvements.
Diffs