Unify O2K_CHECKED_BOUND_ADD_CNS and O2K_VN#128165
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR refactors the JIT's assertion descriptor types by collapsing the two related op2 kinds — O2K_CHECKED_BOUND_ADD_CNS and O2K_VN — into a single, unified O2K_VN_ADD_CNS. The new kind always decomposes op2 as vn + cns, with an explicit IsVNNeverNegative() flag carried separately. This removes a footgun where "checked bound" was previously (incorrectly) treated by some callers as implying non-negativity, and simplifies the assertion-table bookkeeping for VN-vs-VN relop assertions in global propagation.
Changes:
- Replace
O2K_CHECKED_BOUND_ADD_CNSandO2K_VNwith a singleO2K_VN_ADD_CNS; renameGetCheckedBound/GetCheckedBoundConstant/IsCheckedBoundNeverNegativetoGetVN/GetCns/IsVNNeverNegativeand updateCreateForArrLen/CreateCompareCheckedBound/CreateRelopVNandEqualsaccordingly. - Update
RangeCheck::MergeEdgeAssertionsWorkerto use the new accessors and add an explicitIsVNCheckedBound(GetVN())guard on the branch that derives akeBinOpArraylimit (since arbitrary VNs fromCreateRelopVNnow also flow throughO2K_VN_ADD_CNS). - In
optAssertionPropGlobal_RelOp, restrict the direct relop-match tocns == 0forO2K_VN_ADD_CNSand consolidate the duplicate-registration logic inoptAddAssertionto a single branch.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/coreclr/jit/compiler.h |
Unifies the two op2 kinds into O2K_VN_ADD_CNS, renames accessors/fields, updates IsBoundsCheckNoThrow, Equals, and the three factory helpers. |
src/coreclr/jit/assertionprop.cpp |
Updates print/debug/complement/dedup paths to the new kind; adds cns == 0 guard for the direct relop-match in global prop. |
src/coreclr/jit/rangecheck.cpp |
Switches all MergeEdgeAssertionsWorker branches to the new accessors and adds IsVNCheckedBound guard on the keBinOpArray branch to avoid feeding arbitrary VNs into length-shaped Limits. |
|
PTAL @jakobbotsch cc @dotnet/jit-contrib just a clean up, minimal diffs. |
|
/ba-g timeout |
I decided that it makes no sense to have these separate. So now the semantics of
O2K_VN_ADD_CNS:op2.GetVN()being X and CNS beingop2.GetCNS(). Exactly likeO2K_CHECKED_BOUND_ADD_CNSused to be. The decomposition allows us to avoid calling GetVNFunc in the hot loop in MergeEdgeAssertions since we usually are more interested in that X rather than the whole ADD's VN.O2K_VN_ADD_CNSalone doesn't imply that anything is non-negative and no callers assume that (we used to have a bug where IsVNCheckedBound was assumed to be unconditionally never-negative). There isGetOp2().IsVNNeverNegative()which only exists because bound check node actually produce two assertions we can't spawn today because trees may only have 1 assertion. I'm thinking of solving this by introducing an aggregate (or just allow trees to have 2 assertions), or just add a fake COMMA