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

[SkiFoD]

I added the optimization and marked the PR as a draft until it is checked by reviewers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I added the optimization and marked the PR as a draft until it is checked by reviewers.

Author:	SkiFoD
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[jakobbotsch]

It's good practice here to call op1->SetVNsFromNode(add) after this to inherit the VN from the add. Also, I suggest we move this under OptimizationEnabled() below.

[kasperk81]

does it guard against ~x - 1 and cover 1 + ~x?

code

int A(int x) => ~x + 1;
int B(int x) => ~x + 50000;
int C(int x) => 1 + ~x;

int D(int x) => ~x - 1;
int E(int x) => ~x - 50000;
int F(int x) => -1 - ~x;;

expected

A(int):
        mov     eax, edi
        neg     eax
        ret
B(int):
        mov     eax, 49999
        sub     eax, edi
        ret
C(int):
        mov     eax, edi
        neg     eax
        ret
D(int):
        mov     eax, -2
        sub     eax, edi
        ret
E(int):
        mov     eax, -50000
        sub     eax, edi
        ret
F(int):
        mov     eax, edi
        ret

actual

A(int)
        mov     eax, edx
        not     eax
        inc     eax
        ret
B(int)
        mov     eax, edx
        not     eax
        add     eax, 0xc350
        ret
C(int)
        mov     eax, edx
        not     eax
        inc     eax
        ret
D(int):
        mov     eax, edx
        not     eax
        dec     eax
        ret
E(int):
        mov     eax, edx
        not     eax
        add     eax, 0xffff3cb0
        ret
F(int):
        mov     eax, edx
        not     eax
        neg     eax
        dec     eax
        ret

[jakobbotsch]

does it guard against ~x - 1 and cover 1 + ~x?

We generally try to canonicalize constants to the right and restate subtractions with constants in terms of additions with constants.

It seems reasonable to me to handle more constants than 1 here. @SkiFoD, do you want to make that change?

[SkiFoD]

@SkiFoD Sure, @kasperk81 thank you for your notice.

[SkiFoD]

@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?

[jakobbotsch]

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).

[jakobbotsch]

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.

[SkiFoD]

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

[jakobbotsch]

LGTM. Thanks for trying some of the additional variations, even if it didn't turn out to be profitable. I'll launch some additional testing.

[jakobbotsch]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

The failures are known (#69659). @SkiFoD, I assume this is ready?

[jakobbotsch]

Sorry, missed that you had marked this as ready. Thanks!