Creating comma temps differently for SubMulDiv morph - #69770

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

(multiplier * seed2 + increment) % Modulus;

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

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

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

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

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?

Here is the link to the diffs: Diffs

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

Seems there is significant regression introduced on arm64:

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

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

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

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.

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.

Seems like we are not CSEing

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

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