Skip to content

Introduce OAK_*relops*#124030

Merged
EgorBo merged 2 commits intodotnet:mainfrom
EgorBo:oak-relops
Feb 5, 2026
Merged

Introduce OAK_*relops*#124030
EgorBo merged 2 commits intodotnet:mainfrom
EgorBo:oak-relops

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 5, 2026 •
edited
Loading

Today, assertions for relops are represented as relopVN OAK_[NOT]_EQUAL 0 - this forces most assertion consumers to call GetVNFunc over the relopVN to see if one of the operands matches thiers. This PR attempts to remove them and introduce OAK_<relop> so all assertions become just e.g. op1VN OAK_LE op2VN.

The plan is to remove these:

(removed in this PR) O1K_CONSTANT_LOOP_BND
(removed in this PR) O1K_CONSTANT_LOOP_BND_UN
(removed in this PR) O1K_ARR_BND
(will be removed in a follow up) O1K_BOUND_LOOP_BND,    // X < CHECKED_BND
(will be removed in a follow up) O1K_BOUND_OPER_BND,    // X < CHECKED_BND + Y
(removed in this PR) OAK_NO_THROW

Also, no longer weird OAK_NO_THROW without op2. It's now represented as a normal op1VN (idx) OAK_LT_UN op2VN (len)

I need this simplification for some of the optimizations I have in mind (and enable many existing ops for 64-bit integers and floating points). Also, it makes it easier to implement a VN-mapped hash set to quickly validate that we definitely don't have assertions for the given VN.

Diffs - slight improvements in size and TP. I tried to keep the size diffs minimal for the refactoring PR, but it was not possible without ugly hacks to make them exactly zero.

Copilot AI review requested due to automatic review settings February 5, 2026 03:46
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 5, 2026
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a significant refactoring of assertion representation in the JIT compiler, specifically for relational operators. The goal is to simplify how constant bound assertions are stored and processed.

Changes:

  • Adds new assertion kinds for relational operators (OAK_LT, OAK_LE, OAK_GT, OAK_GE and their unsigned variants)
  • Removes O1K_CONSTANT_LOOP_BND and O1K_CONSTANT_LOOP_BND_UN operand kinds
  • Changes assertion storage from "relopVN ==/!= 0" format to direct "vn1 oper vn2" format
  • Adds helper methods for converting between VNFunc and assertion kinds
  • Simplifies CreateConstantLoopBound to extract and store relop components directly

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/coreclr/jit/compiler.h Adds 8 new OAK_* relop kinds, removes 2 O1K_CONSTANT_LOOP_BND kinds, adds VNFuncToKind/KindToOper/ReverseKind helper methods, refactors CreateConstantLoopBound to extract relop components
src/coreclr/jit/assertionprop.cpp Updates optPrintAssertion with new relop kinds, removes O1K_CONSTANT_LOOP_BND handling, simplifies optCreateJTrueBoundsAssertion, updates optAssertionProp_RangeProperties to use new representation
src/coreclr/jit/rangecheck.cpp Simplifies MergeEdgeAssertions to work with new relop representation instead of extracting from stored VN

Copilot AI review requested due to automatic review settings February 5, 2026 04:06
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings February 5, 2026 05:30
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings February 5, 2026 05:59
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings February 5, 2026 06:24
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings February 5, 2026 09:03
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/coreclr/jit/compiler.h:8143

  • The vnBased parameter is not used in this method. This parameter should either be removed if it's no longer needed, or the implementation should be updated to use it if it was previously used for determining comparison logic.
        bool HasSameOp2(const AssertionDsc& that, bool vnBased) const
        {
            if (!GetOp2().KindIs(that.GetOp2().GetKind()))
            {
                return false;
            }

            switch (GetOp2().GetKind())
            {
                case O2K_CONST_INT:
                    return ((GetOp2().GetIntConstant() == that.GetOp2().GetIntConstant()) &&
                            (GetOp2().GetIconFlag() == that.GetOp2().GetIconFlag()));

                case O2K_CONST_DOUBLE:
                    // exact match because of positive and negative zero.
                    return (memcmp(&GetOp2().m_dconVal, &that.GetOp2().m_dconVal, sizeof(double)) == 0);

                case O2K_ZEROOBJ:
                    return true;

                case O2K_BOUND:
                    return GetOp2().GetVN() == that.GetOp2().GetVN();

                case O2K_LCLVAR_COPY:
                    return GetOp2().GetLclNum() == that.GetOp2().GetLclNum();

                case O2K_SUBRANGE:
                    return GetOp2().GetIntegralRange().Equals(that.GetOp2().GetIntegralRange());

                case O2K_INVALID:
                default:
                    assert(!"Unexpected value for GetOp2().m_kind in AssertionDsc.");
                    break;
            }

            return false;
        }

Copilot AI review requested due to automatic review settings February 5, 2026 12:05
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@EgorBo
Copy link
Member Author

EgorBo commented Feb 5, 2026

PTAL @AndyAyersMS @dotnet/jit-contrib

@EgorBo EgorBo marked this pull request as ready for review February 5, 2026 15:50
@EgorBo EgorBo requested review from AndyAyersMS and Copilot February 5, 2026 15:50
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

AndyAyersMS
AndyAyersMS approved these changes Feb 5, 2026
View reviewed changes
@EgorBo EgorBo enabled auto-merge (squash) February 5, 2026 22:58
@EgorBo
Copy link
Member Author

EgorBo commented Feb 5, 2026

/ba-g tvos infra issue

@EgorBo EgorBo merged commit 242ef01 into dotnet:main Feb 5, 2026
131 of 133 checks passed
@EgorBo EgorBo deleted the oak-relops branch February 5, 2026 23:00
@dotnet-maestro dotnet-maestro bot mentioned this pull request Feb 6, 2026
Merged
lewing pushed a commit to lewing/runtime that referenced this pull request Feb 9, 2026
@EgorBo @lewing
Introduce OAK_*relops* (dotnet#124030)
82febbf 
Today, assertions for relops are represented as `relopVN OAK_[NOT]_EQUAL
0` - this forces most assertion consumers to call GetVNFunc over the
relopVN to see if one of the operands matches thiers. This PR attempts
to remove them and introduce `OAK_<relop>` so all assertions become just
e.g. `op1VN OAK_LE op2VN`.

The plan is to remove these:
```
(removed in this PR) O1K_CONSTANT_LOOP_BND
(removed in this PR) O1K_CONSTANT_LOOP_BND_UN
(removed in this PR) O1K_ARR_BND
(will be removed in a follow up) O1K_BOUND_LOOP_BND,    // X < CHECKED_BND
(will be removed in a follow up) O1K_BOUND_OPER_BND,    // X < CHECKED_BND + Y
(removed in this PR) OAK_NO_THROW
```
Also, no longer weird `OAK_NO_THROW` without `op2`. It's now represented
as a normal `op1VN (idx) OAK_LT_UN op2VN (len)`

I need this simplification for some of the optimizations I have in mind
(and enable many existing ops for 64-bit integers and floating points).
Also, it makes it easier to implement a VN-mapped hash set to quickly
validate that we definitely don't have assertions for the given VN.


[Diffs](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1281385&view=ms.vss-build-web.run-extensions-tab)
- slight improvements in size and TP. I tried to keep the size diffs
minimal for the refactoring PR, but it was not possible without ugly
hacks to make them exactly zero.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees

@EgorBo EgorBo

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@EgorBo @AndyAyersMS