JIT: Use faster mod for uint16 values

[MihaZupan]

Closes #111492

• Remember a GTF_UMOD_UINT16_OPERANDS flag during assertion prop
• Expand dividend % constDivisor to (ulong)(dividend * (uint.MaxValue / divisor + 1)) * divisor) >> 32 during lowering

There aren't many diffs, but they look along the lines of

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormatInfo.cs

Lines 2330 to 2331 in 9aa8cbf

	int hashcode = ch % TOKEN_HASH_SIZE;
	int hashProbe = 1 + ch % SECOND_PRIME;

-       mov      edx, eax
-       imul     rdx, rdx, 0xD1FFAB1E
-       shr      rdx, 38
-       imul     edx, edx, 199
-       mov      esi, eax
-       sub      esi, edx
-       mov      dword ptr [rbp-0x34], esi
-       mov      rdx, 0xD1FFAB1E
-       mov      dword ptr [rbp-0x2C], eax
-       mul      rdx:rax, rdx
-       imul     edi, edx, 197
-       mov      eax, dword ptr [rbp-0x2C]
-       sub      eax, edi
+       imul     edi, eax, 0xD1FFAB1E
+       imul     rdx, rdi, 199
+       shr      rdx, 32
+       mov      dword ptr [rbp-0x34], edx
+       imul     edi, eax, 0xD1FFAB1E
+       imul     rax, rdi, 197
+       shr      rax, 32

Example use case that this makes simpler

bool IsWhiteSpace(char c)
{
    ReadOnlySpan<char> HashEntries = [' ', ' ', '\u00A0', ' ', ' ', ' ', ' ', ' ', ' ', '\t', '\n', '\v', '\f', '\r', ' ', ' ', '\u2028', '\u2029', ' ', ' ', ' ', ' ', ' ', '\u202F', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '\u3000', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '\u0085', '\u2000', '\u2001', '\u2002', '\u2003', '\u2004', '\u2005', '\u2006', '\u2007', '\u2008', '\u2009', '\u200A', ' ', ' ', ' ', ' ', ' ', '\u205F', '\u1680', ' ', ' ', ' ', ' ', ' ', ' '];
    return HashEntries[c % HashEntries.Length] == c;
}

[jakobbotsch]

This flag needs to be checked by GenTree::Compare (reason N+1 that flags changing IR semantics is unfortunate IR design)

[EgorBo]

I don't think we can just avoid it (not here, but in general). But perhaps we can introduce a separate Enum with changing-semantics flags or something like that

[SingleAccretion]

I don't see why this would need to be taken into account in GenTree::Compare. This flag, similar to GTF_IND_NONFAULTING, only records an optimization fact.

[jakobbotsch]

Ideally operations with different semantics should have different gtOper, like e.g. BSWAP and BSWAP16 do. That might not be as convenient to do in some cases, but ideally we should optimize the process of adding new types of nodes to be as painless as possible.

[jakobbotsch]

I don't see why this would need to be taken into account in GenTree::Compare. This flag, similar to GTF_IND_NONFAULTING, only records an optimization fact.

For example head-merging a 16-bit division with a non-16-bit division would not be a legal transformation.

[SingleAccretion]

I don't disagree that a better way to handle this would be to introduce a GT_UMOD16 that would perform true 16-bit division, ignoring the upper bits of the operands.

But in this flags model, there is no true 16 division, only 32 bit division "for which we know the operands fit into a certain range". It would be invalid IR for a node tagged with the flag to appear where the dynamic range of the operands does not actually fit. Hence, Compare ought not to care.

What needs to care is all the places that reuse UMOD nodes.

[jakobbotsch]

It would be invalid IR for a node tagged with the flag to appear where the dynamic range of the operands does not actually fit.

Yes, and this can happen in head-merging if it merges a 16-bit division and a non-16-bit division and moves them across a guard on the divisor range. I view that as changing the semantics of the node (the domain of the operands is TYP_INT, yet the value computed is different for a divisor outside [0..0xffff]).

[jakobbotsch]

I do not think we can model this as an optimization fact. It requires us to set GTF_ORDER_SIDEEFF on both this node and its guarding condition IR node, similar to other unmodelled control-flow dependencies we have.

[SingleAccretion]

Hmm, I see what you are saying (that this flag should mean "a kind of true 16 bit division"). Perhaps it could use a better name then.

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).