Creating comma temps differently for SubMulDiv morph

[TIHan]

Description

This changes how comma temps are created for SubMulDiv morph.

Example code:

(multiplier * seed2 + increment)% Modulus;

In morph, this would turn into:

(lettmp1= multiplier * seed2 + increment)in tmp1)-(tmp1 / Modulus)* Modulus

This PR morphs it into:

lettmp1= multiplier * seed2 + increment in
tmp1 -(tmp1 / Modulus)* Modulus

Basically, we are moving the comma temps to be the root of the expression instead of leafs.

Acceptance Criteria

• CI passes
• Investigate a possible generic way of lifting GT_COMMAs up to be root nodes instead of leaf nodes.
  • This might be a bit harder to do in a generic way. For now, let's not worry about it.

Diffs

[ghost]

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

Issue Details

---

At the moment, this is an experiment.

This changes how comma temps are created for SubMulDiv morph.

Example code:

(multiplier * seed2 + increment)% Modulus;

In morph, this would turn into:

(lettmp1= multiplier * seed2 + increment)in tmp1)-(tmp1 / Modulus)* Modulus

This PR morphs it into:

lettmp1= multiplier * seed2 + increment in
tmp1 -(tmp1 / Modulus)* Modulus

Author:	TIHan
Assignees:	TIHan
Labels:	area-CodeGen-coreclr
Milestone:	-

[TIHan]

@dotnet/jit-contrib This is ready. The diffs will show some regressions, but they are not detrimental. The improvements on both x64 and ARM64 look good, especially ARM64.

[TIHan]

@kunalspathak@AndyAyersMS - I feel good about this change and it's ready. It should help optimizing 'mod' since I'll be able to do the transformation before rationalization.

[TIHan]

@kunalspathak@AndyAyersMS - I feel good about this change and it's ready. It should help optimizing 'mod' since I'll be able to do the transformation before rationalization.

[AndyAyersMS]

Looks good.

(but: you have formatting to fix).

[JulieLeeMSFT]

The diffs will show some regressions, but they are not detrimental. The improvements on both x64 and ARM64 look good, especially ARM64.

@TIHan can you show the perf results in this PR?

[TIHan]

Here is the link to the diffs: Diffs

[kunalspathak]

Seems there is significant regression introduced on arm64:

 1380 (0.88 % of base) : 4937.dasm - u8rem:TestEntryPoint():int
1340 (0.80 % of base) : 4917.dasm - i8rem:TestEntryPoint():int
976 (0.59 % of base) : 264436.dasm - u4rem:TestEntryPoint():int
128 (0.24 % of base) : 16098.dasm - lclfldrem:Main():int
16 (4.12 % of base) : 173626.dasm - Test_10w5d.testout1:Func_0_1_1_6_2():double
16 (4.12 % of base) : 176286.dasm - Test_10w5d.testout1:Func_0_1_1_6_2():double
12 (0.81 % of base) : 58712.dasm - ILGEN_0x13230206:Method_0xce57b468(long,double,byte,long):int
12 (1.58 % of base) : 76667.dasm - ILGEN_0x3aa9c940:main():int
12 (0.36 % of base) : 45954.dasm - ILGEN_0x977f9ed2:Method_0xf6b7353b():float
8 (4.35 % of base) : 78111.dasm - DevDiv_590771:ILGEN_METHOD(long,ushort,long,int,ushort,long):byte
4 (1.39 % of base) : 77616.dasm - ILGEN_0x152f1077:Method_0x2763af56(long):int

[kunalspathak]

Seems there is significant regression introduced on arm64:

just noticed your message about the regression in #69770 (comment)

[kunalspathak]

Seems like we are not CSEing the % operation for this:

publicPacket256Tracer(int width,int height){if((width%VectorPacket256.Packet256Size)!=0){width+=VectorPacket256.Packet256Size-(width%VectorPacket256.Packet256Size);}Width=width;Height=height;}

[kunalspathak]

Something doesn't feel right in Before: tree. Did it mean to have (V00 * V00) % V01. Same in After:.

[TIHan]

(V00 * V00) % V01 is intentional.

In the after tree, you see that it takes (V00 * V00) and hoists it out:

// +--* ASG int
// | +--* LCL_VAR int V03 tmp1
// | \--* MUL int
// | +--* LCL_VAR int V00 arg0
// | \--* LCL_VAR int V00 arg0

[kunalspathak]

As spoke offline, change MUL to BINOP and add a note that it is applicable for any BINOP that is safe to clone.

[TIHan]

Seems there is significant regression introduced on arm64:

It does look significant, but when I looked at the top regressions, the actual code is really wild with a lot of casts and checked contexts - something you really would not see in user code.

[TIHan]

Seems like we are not CSEing the % operation for this:

That is a good catch. I'm surprised by this one - I'm even more surprised how it was able to CSE it before because one of the ops would have a GT_COMMA in it.

[kunalspathak]

LGTM

[kunalspathak]

Seems like we are not CSEing

My before and after were flipped. I think it looks good.

[TIHan]

As a note from @kunalspathak 's comment regarding the % CSE in this image:
https://user-images.githubusercontent.com/12488060/171420114-f42a8c64-54ca-476f-a9c8-43b13c6775fe.png
It is actually reversed; the left side is the diff whereas the right side was the base. This means that the change actually makes CSE work in this example :)