Skip to content

JIT: Fold "cmp & 1" to "cmp" #52524

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

Merged
merged 1 commit into from
May 10, 2021
Merged

JIT: Fold "cmp & 1" to "cmp" #52524

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14090,6 +14090,14 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
op2 = tree->AsOp()->gtOp2;
}

// Fold "cmp & 1" to just "cmp"
if (tree->OperIs(GT_AND) && tree->TypeIs(TYP_INT) && op1->OperIsCompare() && op2->IsIntegralConst(1))
Copy link
Contributor

@sandreenko sandreenko May 10, 2021

Choose a reason for hiding this comment

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

sorry, I am late for the review, two questions:

  1. do we want to do it when optimizations are disabled?
  2. Can op2 or tree be a CSE candidates that we destroy while CSE is keeping links on them?

Copy link
Member Author

@EgorBo EgorBo May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. I found plenty of such small peepholes in morph without OptimizationEnabled guard so it wasn't clear to me when we should really apply it - I guess we want to eventually have three different modes: MinOpts, tier0 (simple opts) and tier1 (all opts).

  2. op2 is always an integer const (1) and CSE doesn't work for them without a special switch -- so it's not a correct behavior to delete a potential CSE candidate and it might lead to a crash/assert?

Copy link
Contributor

@sandreenko sandreenko May 12, 2021

Choose a reason for hiding this comment

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

op2 is always an integer const (1) and CSE doesn't work for them without a special switch -- so it's not a correct behavior to delete a potential CSE candidate and it might lead to a crash/assert?

the switch is on for arm64 but tree can be a CSE candidate without it. I would recommend adding a change to protect against it.

{
DEBUG_DESTROY_NODE(op2);
DEBUG_DESTROY_NODE(tree);
return op1;
}

// See if we can fold floating point operations (can regress minopts mode)
if (opts.OptimizationEnabled() && varTypeIsFloating(tree->TypeGet()) && !optValnumCSE_phase)
{
Expand Down