Skip to content

JIT: Enable fgOptimizeRelationalComparisonWithCasts for EQ/NE #128091

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

Open
BoyBaykiller wants to merge 3 commits into
base: main
Choose a base branch
from
+3 −6
Open

JIT: Enable fgOptimizeRelationalComparisonWithCasts for EQ/NE #128091

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4654abb
* enable for all OperIsCmpCompare
BoyBaykiller May 21, 2026
a7e9219
* update assert to support EQ/NE type rules
BoyBaykiller May 24, 2026
a0f04cc
* undo assert change as the underlying issue was faulty test
BoyBaykiller May 24, 2026
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
12 changes: 5 additions & 7 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7862,9 +7862,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, bool* optAssertionPropDone)
case GT_GE:
case GT_GT:
{
assert(tree->OperIsCmpCompare());
fgPushConstantsRight(tree->AsOp());

tree = fgOptimizeRelationalComparison(tree->AsOp());
if (!tree->OperIsBinary())
{
Expand Down Expand Up @@ -8783,11 +8780,11 @@ GenTree* Compiler::fgOptimizeRelationalComparison(GenTreeOp* cmp)
{
assert(cmp->OperIsCmpCompare());

fgPushConstantsRight(cmp);

GenTree* tree = cmp;

// TODO-CQ: Should be called for all comparisons
if (tree->OperIs(GT_LT, GT_LE, GT_GE, GT_GT) &&
(tree->gtGetOp1()->OperIs(GT_CAST) || tree->gtGetOp2()->OperIs(GT_CAST)))
if (tree->OperIsCmpCompare() && (tree->gtGetOp1()->OperIs(GT_CAST) || tree->gtGetOp2()->OperIs(GT_CAST)))
Copy link
Copy Markdown
Contributor Author

@BoyBaykiller BoyBaykiller May 21, 2026

Choose a reason for hiding this comment

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

This is the (only) meaningful change.

{
tree = fgOptimizeRelationalComparisonWithCasts(tree->AsOp());
}
Expand Down Expand Up @@ -10945,7 +10942,6 @@ GenTree* Compiler::fgOptimizeRelationalComparisonWithCasts(GenTreeOp* cmp)
{
assert(cmp->OperIsCmpCompare());
assert(cmp->gtGetOp1()->OperIs(GT_CAST) || cmp->gtGetOp2()->OperIs(GT_CAST));
assert(genActualType(cmp->gtGetOp1()) == genActualType(cmp->gtGetOp2()));
Copy link
Copy Markdown
Member

@EgorBo EgorBo May 21, 2026

Choose a reason for hiding this comment

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

why? if you think this assert is incorrect you should fix it, not move it to somewhere to silence it

Copy link
Copy Markdown
Contributor Author

@BoyBaykiller BoyBaykiller May 21, 2026

Choose a reason for hiding this comment

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

Hm, doesn't it make sense to move it after the supportedOp check

Copy link
Copy Markdown
Contributor Author

@BoyBaykiller BoyBaykiller May 24, 2026

Choose a reason for hiding this comment

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

@EgorBo I updated the assert to implement the EQ/NE type rules.


GenTree* op1 = cmp->gtGetOp1();
GenTree* op2 = cmp->gtGetOp2();
Expand Down Expand Up @@ -10985,6 +10981,8 @@ GenTree* Compiler::fgOptimizeRelationalComparisonWithCasts(GenTreeOp* cmp)
return cmp;
}

assert(genActualType(cmp->gtGetOp1()) == genActualType(cmp->gtGetOp2()));
Copy link
Copy Markdown
Contributor Author

@BoyBaykiller BoyBaykiller May 21, 2026

Choose a reason for hiding this comment

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

Reason I moved this down:

When calling this function for EQ/NE this assert fired in some coreclr_tests. And moving it after the supportedOp check fixes it. This is the tree it failed on:

[000030] --CXG------                         *  EQ        int   
[000013] --CXG+-----                         +--*  CALL help ref    CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE_MAYBENULL
[000469] n---G+----- arg0 rcx                |  \--*  IND       byref 
[000468] -----+-----                         |     \--*  ADD       byref 
[000011] -----+-----                         |        +--*  LCL_VAR   byref  V01 arg1         
[000467] -----+-----                         |        \--*  CNS_INT   long   8
[000029] ----G+---U-                         \--*  CAST      long <- uint
[000470] ----G+-----                            \--*  EQ        int   
[000473] n---G+-----                               +--*  IND       byref 
[000472] -----+-----                               |  \--*  ADD       byref 
[000018] -----+-----                               |     +--*  LCL_VAR   byref  V02 arg2         
[000471] -----+-----                               |     \--*  CNS_INT   long   8
[000476] n---G+-----                               \--*  IND       byref 
[000475] -----+-----                                  \--*  ADD       byref 
[000025] -----+-----                                     +--*  LCL_VAR   byref  V03 arg3         
[000474] -----+-----                                     \--*  CNS_INT   long   8


auto isUpperZero = [this](GenTree* op) {
if (op->IsIntegralConst())
{
Expand Down
Loading