Skip to content
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

Fold null checks against known non-null values #109164

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+41 −35
Open

Fold null checks against known non-null values #109164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c76e4e1
Fold null checks against known non-null values
MichalPetryka Oct 23, 2024
90b0762
Update gentree.cpp
MichalPetryka Oct 23, 2024
560a826
Update gentree.cpp
MichalPetryka Oct 23, 2024
9260b88
Update flowgraph.cpp
MichalPetryka Oct 28, 2024
365b604
Update gentree.cpp
MichalPetryka Oct 28, 2024
fddb0b3
Merge branch 'main' into patch-43
MichalPetryka Nov 21, 2024
882ba2c
Merge branch 'main' into patch-43
MichalPetryka Nov 22, 2024
ebf1479
Merge branch 'main' into patch-43
EgorBo Dec 16, 2024
a74ab04
Merge branch 'main' into patch-43
MichalPetryka Jan 6, 2025
fd89299
Merge branch 'main' into patch-43
EgorBo Jan 27, 2025
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
3 changes: 3 additions & 0 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,9 @@ bool Compiler::fgAddrCouldBeNull(GenTree* addr)
case GT_ARR_ADDR:
return (addr->gtFlags & GTF_ARR_ADDR_NONNULL) == 0;

case GT_BOX:
return !addr->IsBoxedValue();
Copy link
Member

Choose a reason for hiding this comment

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

remind me why GT_BOX may produce null possibly? I don't understand the need in IsBoxedValue()

Copy link
Contributor Author

@MichalPetryka MichalPetryka Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IsBoxedValue checks the GTF_BOX_VALUE flag which is documented as guaranteeing that the box is not null, when we discussed this on Discord we reached the conclusion that the flag might be an old leftover that's not needed anymore and could be removed, but like I said there, I'd prefer such cleanup to be a separate PR.


case GT_LCL_VAR:
return !lvaIsImplicitByRefLocal(addr->AsLclVar()->GetLclNum());

Expand Down
73 changes: 38 additions & 35 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14490,9 +14490,10 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
// Optimize boxed value classes; these are always false. This IL is
// generated when a generic value is tested against null:
// <T> ... foo(T x) { ... if ((object)x == null) ...
if ((val == 0) && op->IsBoxedValue())
// Also fold checks against known non-null data like static readonlys
if ((val == 0) && !fgAddrCouldBeNull(op))
{
JITDUMP("\nAttempting to optimize BOX(valueType) %s null [%06u]\n", GenTree::OpName(oper),
JITDUMP("\nAttempting to optimize BOX(valueType)/non-null %s null [%06u]\n", GenTree::OpName(oper),
dspTreeID(tree));

// We don't expect GT_GT with signed compares, and we
Expand All @@ -14504,44 +14505,46 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
}
else
{
// The tree under the box must be side effect free
// since we will drop it if we optimize.
assert(!gtTreeHasSideEffects(op->AsBox()->BoxOp(), GTF_SIDE_EFFECT));
bool wrapEffects = true;
if (op->IsBoxedValue())
{
// The tree under the box must be side effect free
// since we will drop it if we optimize.
assert(!gtTreeHasSideEffects(op->AsBox()->BoxOp(), GTF_SIDE_EFFECT));

// See if we can optimize away the box and related statements.
GenTree* boxSourceTree = gtTryRemoveBoxUpstreamEffects(op);
bool didOptimize = (boxSourceTree != nullptr);
// See if we can optimize away the box and related statements.
wrapEffects = (gtTryRemoveBoxUpstreamEffects(op) == nullptr);
}

// If optimization succeeded, remove the box.
if (didOptimize)
// Set up the result of the compare.
int compareResult;
if (oper == GT_GT)
{
// Set up the result of the compare.
int compareResult = 0;
if (oper == GT_GT)
{
// GT_GT(null, box) == false
// GT_GT(box, null) == true
compareResult = (op1 == op);
}
else if (oper == GT_EQ)
{
// GT_EQ(box, null) == false
// GT_EQ(null, box) == false
compareResult = 0;
}
else
{
assert(oper == GT_NE);
// GT_NE(box, null) == true
// GT_NE(null, box) == true
compareResult = 1;
}

JITDUMP("\nSuccess: replacing BOX(valueType) %s null with %d\n", GenTree::OpName(oper),
compareResult);
// GT_GT(null, op) == false
// GT_GT(op, null) == true
compareResult = (op1 == op);
}
else if (oper == GT_EQ)
{
// GT_EQ(op, null) == false
// GT_EQ(null, op) == false
compareResult = 0;
}
else
{
assert(oper == GT_NE);
// GT_NE(op, null) == true
// GT_NE(null, op) == true
compareResult = 1;
}

return NewMorphedIntConNode(compareResult);
GenTree* newTree = gtNewIconNode(compareResult);
if (wrapEffects)
{
newTree = gtWrapWithSideEffects(newTree, op, GTF_ALL_EFFECT);
}
op = newTree;
goto DONE_FOLD;
}
}
else
Expand Down
Loading