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

Jump to bottom

Optimization for "~x + 1" to "-x" (#69003) #69600

Merged
merged 4 commits into from
Jun 7, 2022
Merged

Optimization for "~x + 1" to "-x" (#69003) #69600

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c40ab69
Optimization for "~x + 1" to "-x" (#69003)
SkiFoD May 20, 2022
bc447a7
op1 now inherits VNs from add node (#69003)
SkiFoD May 20, 2022
326d03b
Add optimizations for other constants (#69003)
SkiFoD May 21, 2022
62b3dc0
Revert changes made for constant support (#69003)
SkiFoD May 23, 2022
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
33 changes: 33 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 @@ -12913,6 +12913,39 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add)
DEBUG_DESTROY_NODE(add);
return op1;
}

// Fold (~x + GT_CNS_INT/GT_CNS_LNG) to ((GT_CNS_INT/GT_CNS_LNG - 1) - x).
// Ex. (~x + 2) = (1 - x)
if (op1->OperIs(GT_NOT) && op2->IsIntegralConst() &&
(op2->AsIntCon()->IconValue() > 1 || op2->AsIntCon()->IconValue() < 1))
{

add->SetOper(GT_SUB);

add->gtOp1 = op2;
GenTreeIntCon* constOp2 = op2->AsIntCon();
constOp2->SetValueTruncating(constOp2->IconValue() - 1);

add->gtOp2 = op1->AsOp()->gtGetOp1();
DEBUG_DESTROY_NODE(op1);

return add;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakobbotsch I added this code to support compilation of (~x + 2) as shown here https://godbolt.org/z/TqTfq3dEq
I don't like this part tho, because it generates worse diffs and I don't like that I have to to put Const Node to the left.
Diff with this code
Diff without this code
What do you think of this part of code? Have I missed anything which makes the diffs worse?

Copy link
Member

Choose a reason for hiding this comment

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

I can see that other code in the JIT leaves -a + C alone:

runtime/src/coreclr/jit/morph.cpp

Lines 12883 to 12893 in f832309

// Do not do this if "op2" is constant for canonicalization purposes.
if (op1->OperIs(GT_NEG) && !op2->OperIs(GT_NEG) && !op2->IsIntegralConst() && gtCanSwapOrder(op1, op2))
{
add->SetOper(GT_SUB);
add->gtOp1 = op2;
add->gtOp2 = op1->AsOp()->gtGetOp1();
DEBUG_DESTROY_NODE(op1);
return add;
}

I would do the same here to avoid introducing this pattern with a constant on the left.
Besides, it is not clear that C - x is always preferred over ~x + C as it requires an extra register on x86/x64. The JIT does not have mechanism for picking between two forms like this depending on register pressure.
So I think you should go back to just optimizing ~x + 1. You can also experiment with ~x + C => -x + (C - 1) and see if that unlocks further optimizations (you can check occurrences in SPMI).


// Fold (-1 - (~x)) to x
if ((op1->OperIs(GT_NEG) && op1->AsOp()->gtGetOp1()->OperIs(GT_NOT)) &&
op2->IsIntegralConst(-1))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to do this as two separate transforms -(~x) => x + 1 and ~(-x) => x - 1. However, I think you should check if there are occurrences in SPMI to see if we should bother with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had played with it and tried different variations, but there were not any occurrences in SPMI, so I reverted the changes.

{
GenTree* res = op1->AsOp()->gtGetOp1()->AsOp()->gtGetOp1();
res->SetVNsFromNode(add);

DEBUG_DESTROY_NODE(op1);
DEBUG_DESTROY_NODE(op2);
DEBUG_DESTROY_NODE(add);
return res;
}


}

return nullptr;
Expand Down