Remove some unneeded code from division morphing

[SingleAccretion]

Remove GTF_UNSIGNED check from the condition

It is not necessary: GTF_UNSIGNED does not have anything
to do with the operands being unsigned.

Some positive diffs in runtime tests for win-x86 and
one regression in System.Net.WebSockets.ManagedWebSocket.ApplyMask.
The regression is because we generate two "div"s for a long UMOD
on x86 with a constant divisor, always, even for powers of two.
Something to improve for sure (and I have a fix for it in the pipeline).

The diffs

Running asm diffs of coreclr_tests.pmi.windows.x86.checked.mch

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 11904
Total bytes of diff: 11564
Total bytes of delta: -340 (-2.86% of base)
    diff is an improvement.


Top method regressions (bytes):
           6 ( 2.78% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
           6 ( 2.78% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
           1 ( 0.46% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int

Top method improvements (bytes):
        -104 (-5.68% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
        -104 (-7.28% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -77 (-5.95% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -22 (-1.45% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -22 (-1.45% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -12 (-0.66% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -12 (-0.66% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int

Top method regressions (percentages):
           6 ( 2.78% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
           6 ( 2.78% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
           1 ( 0.46% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int

Top method improvements (percentages):
        -104 (-7.28% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -77 (-5.95% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
        -104 (-5.68% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -22 (-1.45% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -22 (-1.45% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -12 (-0.66% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -12 (-0.66% of base) - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int

10 total methods with Code Size differences (7 improved, 3 regressed), 0 unchanged.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 417
Total bytes of diff: 468
Total bytes of delta: 51 (12.23% of base)
    diff is a regression.


Top method regressions (bytes):
          51 (12.23% of base) - System.Net.WebSockets.ManagedWebSocket:ApplyMask(System.Span`1[Byte],int,int):int

Top method regressions (percentages):
          51 (12.23% of base) - System.Net.WebSockets.ManagedWebSocket:ApplyMask(System.Span`1[Byte],int,int):int

1 total methods with Code Size differences (0 improved, 1 regressed), 0 unchanged.

Naturally, no diffs for win-x64, linux-x64 or linux-arm.

Don't fold casts from constants in UMOD morphing

It used to be that ldc.i4.1 conv.i8 sequences survived
importation, and since division morphing is sensitive to
constant divisors, morph tried to fold them. This is
no longer the case, so stop doing that.

Of couse, morph can be called from anywhere at any
point, but if some code is creating casts from constants,
the proper place to fix is that code.

No diffs for win-x86 or win-x64 or linux-arm.

There is one more place in morph (GTF_MUL_64RSLT recognition) that needs fixing for the same reasons as above, but that one actually has CQ (I regressed it with #47133) and correctness (it can lead to silent bad codegen) implications, so it will be a separate PR.

cc @sandreenko

[sandreenko]

PTAL @dotnet/jit-contrib

[JulieLeeMSFT]

@kunalspathak PTAL.

[kunalspathak]

It used to be that ldc.i4.1 conv.i8 sequences survived
importation, and since division morphing is sensitive to
constant divisors, morph tried to fold them. This is
no longer the case, so stop doing that.

I am not too familiar with this code, so could you please provide a context on why the sequence ldc.i4.1 conv.i8 doesn't survive importation anymore?

[SingleAccretion]

I am not too familiar with this code, so could you please provide a context on why the sequence ldc.i4.1 conv.i8 doesn't survive importation anymore?

We fold it:

runtime/src/coreclr/jit/importer.cpp

Lines 13543 to 13547 in 8103a7e

	if (op1->gtGetOp1()->OperIsConst() && opts.OptimizationEnabled())
	{
	// Try and fold the introduced cast
	op1 = gtFoldExprConst(op1);
	}

[kunalspathak]

It is not necessary: GTF_UNSIGNED does not have anything
to do with the operands being unsigned.

From the GTF_UNSIGNED definition, it looks like it tells that the operands are unsigned. Could you please elaborate what exactly you mean by "GTF_UNSIGNED does not have anything to do with the operands being unsigned."?

runtime/src/coreclr/jit/gentree.h

Lines 403 to 404 in 8103a7e

	GTF_UNSIGNED = 0x00008000, // With GT_CAST: the source operand is an unsigned type
	// With operators: the specified node is an unsigned operator

[kunalspathak]

actually has CQ (I regressed it with #47133) and correctness (it can lead to silent bad codegen) implications,

Do you have an issue open for the correctness problem?

[SingleAccretion]

Do you have an issue open for the correctness problem?

I fixed it in #53566.

[kunalspathak]

Minor comments/questions.

[SingleAccretion]

From the GTF_UNSIGNED definition, it looks like it tells that the operands are unsigned. Could you please elaborate what exactly you mean by "GTF_UNSIGNED does not have anything to do with the operands being unsigned."?

Sure. The code in question is checking the "signedness" of UMOD's operands, not the "signedness" of operands of UMOD's operands, which the flag does indeed determine.

(In general though, I would say that the concept of "signedness" does not apply in IR - all nodes have no sign, they just produce bits. GTF_UNSIGNED decides how those bits are interpreted)