Draft for x ^ 0 opt (#65071) #65072

SkiFoD · 2022-02-09T12:34:52Z

ghost · 2022-02-09T12:34:59Z

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

Issue Details

Author:	SkiFoD
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2022-02-09T12:51:27Z

src/coreclr/jit/lowerxarch.cpp

+
+            return op1;
+        }
+    }


Consider moving to morph, e.g. x*0 -> 0:

runtime/src/coreclr/jit/morph.cpp

Lines 13723 to 13733 in fe24ab3

if (mult == 0)

{

// We may be able to throw away op1 (unless it has side-effects)

if ((op1->gtFlags & GTF_SIDE_EFFECT) == 0)

{

DEBUG_DESTROY_NODE(op1);

DEBUG_DESTROY_NODE(mul);

return op2; // Just return the "0" node

}

in theory you can also improve this x * 0 by not giving up on side effects and making a COMMA but it's up to you.
Just make sure you don't drop CSE candidates

in theory you can also improve this x * 0 by not giving up on side effects and making a COMMA but it's up to you. Just make sure you don't drop CSE candidates

Could you give me more details about this? I'm a very newbie in the project and a lot of obvious things are not that obvious for me yet😊

Don't worry, none of those things were obvious, :)

EgorBo · 2022-02-09T12:56:02Z

Btw, we already have X ^ 0 -> X in morph, but for Vectors 😄

src/coreclr/jit/morph.cpp

SingleAccretion · 2022-02-09T17:38:08Z

I took a look at the regressions. They are caused by missing CSEs of the following form:

-CSE candidate #01, key=$40 in BB01, [cost= 3, size= 3]:
-N003 (  3,  3) CSE #01 (use)[000087] ------------              *  XOR       int    <l:$40, c:$483>
-N001 (  1,  1)              [000085] ------------              +--*  LCL_VAR   int    V07 tmp3         u:2 (last use) <l:$40, c:$483>
-N002 (  1,  1)              [000761] ------------              \--*  CNS_INT   int    0 $40

Apparently those help the RA allocate more optimally (even as otherwise they are clearly not useful - the XOR is a NOP).

(Notably, not always - we have some improvements in these tests as well)

Seeing as the diffs are in basically one test that has been duplicated via code generation, I personally think it would be ok to accept the regressions while leaving the optimization in morph. It should help CSE (and the rest of the frontend) make better decisions in other situations.

src/coreclr/jit/morph.cpp

SingleAccretion · 2022-02-09T17:40:30Z

src/coreclr/jit/morph.cpp

+//   The optimized tree, currently always a local variable, in case any transformations
+//   were performed. Otherwise, "nullptr", guaranteeing no state change.
+//
+GenTree* Compiler::fgOptimizeBitwiseXor(GenTreeOp* xorOp)


You could leave a TODO-Cleanup: move the optimizations from "fgMorphSmpOpOptional" to this method..

I moved the xor related code into this method and noticed that the condition of this branch looks odd to me.

runtime/src/coreclr/jit/morph.cpp

Lines 13901 to 13909 in 3449312

else if (op2->IsIntegralConst(1) && op1->OperIsCompare())

{

/* "binaryVal ^ 1" is "!binaryVal" */

gtReverseCond(op1);

DEBUG_DESTROY_NODE(op2);

DEBUG_DESTROY_NODE(xorOp);

return op1;

}

Is it actually possible for a child node of XOR node to be a compare node? If I'm not wrong then it is a good candidate to be removed and extend the previous condition with || op2->IsIntegralConst(1) since "x ^ -1 " = "~x" and "x ^ 1" = "~x". Correct me if I'm wrong :)

Is it actually possible for a child node of XOR node to be a compare node?

Why not?

ldarg 0 ldarg 1 ceq ldc.i4 1 xor

Would produce a tree that this is looking for. Not sure how often this is hit in practice (you can remove it and see what SPMI tells, I would guess we'll see lots of such patterns in auto-generated IL in tests).

Yeah, you are right. Removing the conditions emits 2 small regressions. What do you think of "x ^ 1" = "~x"? A little investigation gives me this diff
diff_summary.17.md
Doesn't look good, does it?

What do you think of "x ^ 1" = "~x"?

Are you suggesting to apply it for any x, not just compares? But that would not be correct, it only affects the lowest bit.

All right. Thank you.

SingleAccretion · 2022-02-10T20:43:26Z

The code changes LGTM. However, it seems there are real regressions on 32 bit platforms, I took a look - they're all CSE-induced as well. It seems we're better of with CSEing a TYP_LONG LCL_VAR-equivalents with decomposition in the picture (not totally sure why - didn't have time to dig deep).

@dotnet/jit-contrib for opinions.

EgorBo · 2022-02-21T10:32:24Z

@SkiFoD sorry for the delay, could you please rebase your PR against latest Main? so we can kick spmi once again

EgorBo · 2022-03-14T10:49:20Z

@SkiFoD I'm really sorry, was distracted a bit - could you please resolve the conflict and tag me back so we can merge it?

…iseXor" (dotnet#65071)

SkiFoD · 2022-03-15T08:28:15Z

@EgorBo I updated the branch and run SPMI once again. I got confussed by the results so I'd like to know your opinion on them.

src/coreclr/jit/morph.cpp

EgorBo · 2022-03-15T10:58:15Z

src/coreclr/jit/morph.cpp

+    else if (op2->IsIntegralConst(1) && op1->OperIsCompare())
+    {
+        /* "binaryVal ^ 1" is "!binaryVal" */
+        gtReverseCond(op1);


I realize that it's not your code but it looks weird that it doesn't use return value

Ah, I see it modifies the node if it's OperIsCompare

EgorBo · 2022-03-15T10:59:25Z

Looks good to me, thanks! let's wait for the last job to finish

EgorBo · 2022-03-28T10:41:54Z

@SkiFoD thank you for the contribution!

SkiFoD · 2022-03-28T10:44:41Z

@SingleAccretion @EgorBo Thank you both for guiding me through. Learnt a lot, Pleasure to work with you.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 9, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

EgorBo reviewed Feb 9, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Feb 9, 2022

View reviewed changes

SkiFoD marked this pull request as ready for review February 10, 2022 11:45

SkiFoD requested review from EgorBo, SingleAccretion and Wraith2 February 10, 2022 18:18

Wraith2 approved these changes Feb 10, 2022

View reviewed changes

JulieLeeMSFT assigned EgorBo and SkiFoD Feb 16, 2022

JulieLeeMSFT added this to the 7.0.0 milestone Feb 16, 2022

SkiFoD added 4 commits March 15, 2022 11:10

Draft for x ^ 0 opt (dotnet#65071)

ace3a7f

Moved the optimization to morph (dotnet#65071)

66f765f

Removed unessesery "gtIsActiveCSE_Candidate" checks (dotnet#65701)

1568ee7

Moved XOR related code from "fgMorphSmpOpOptional" to "fgOptimizeBitw…

c7cdc5c

…iseXor" (dotnet#65071)

SkiFoD force-pushed the skifod/issue-65071 branch from 1491f41 to c7cdc5c Compare March 15, 2022 08:25

EgorBo reviewed Mar 15, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

EgorBo reviewed Mar 15, 2022

View reviewed changes

EgorBo approved these changes Mar 15, 2022

View reviewed changes

Made a suggested change (dotnet#65071)

17e0f90

EgorBo merged commit 071e772 into dotnet:main Mar 28, 2022

ghost locked as resolved and limited conversation to collaborators Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft for x ^ 0 opt (#65071) #65072

Draft for x ^ 0 opt (#65071) #65072

SkiFoD commented Feb 9, 2022

ghost commented Feb 9, 2022

EgorBo Feb 9, 2022

EgorBo Feb 9, 2022

SkiFoD Feb 9, 2022

Wraith2 Feb 9, 2022

EgorBo commented Feb 9, 2022

SingleAccretion commented Feb 9, 2022

SingleAccretion Feb 9, 2022

SkiFoD Feb 10, 2022

SingleAccretion Feb 10, 2022

SkiFoD Feb 10, 2022

SingleAccretion Feb 10, 2022

SkiFoD Feb 10, 2022

SingleAccretion commented Feb 10, 2022

EgorBo commented Feb 21, 2022

EgorBo commented Mar 14, 2022

SkiFoD commented Mar 15, 2022

EgorBo Mar 15, 2022

EgorBo Mar 15, 2022

EgorBo commented Mar 15, 2022

EgorBo commented Mar 28, 2022

SkiFoD commented Mar 28, 2022

	if (mult == 0)
	{
	// We may be able to throw away op1 (unless it has side-effects)

	if ((op1->gtFlags & GTF_SIDE_EFFECT) == 0)
	{
	DEBUG_DESTROY_NODE(op1);
	DEBUG_DESTROY_NODE(mul);

	return op2; // Just return the "0" node
	}

	else if (op2->IsIntegralConst(1) && op1->OperIsCompare())
	{
	/* "binaryVal ^ 1" is "!binaryVal" */
	gtReverseCond(op1);
	DEBUG_DESTROY_NODE(op2);
	DEBUG_DESTROY_NODE(xorOp);

	return op1;
	}

Draft for x ^ 0 opt (#65071) #65072

Draft for x ^ 0 opt (#65071) #65072

Conversation

SkiFoD commented Feb 9, 2022

ghost commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo commented Feb 9, 2022

SingleAccretion commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SingleAccretion commented Feb 10, 2022

EgorBo commented Feb 21, 2022

EgorBo commented Mar 14, 2022

SkiFoD commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo commented Mar 15, 2022

EgorBo commented Mar 28, 2022

SkiFoD commented Mar 28, 2022