fix: expand GT_UMOD with constant divisor in morph to enable CSE

[mad1081]

Fixes #119131

Related:

• JIT: Avoid reordering operands in fgMorphModToSubMulDiv #71615 — fgMorphModToSubMulDiv was already made to handle GT_UMOD correctly
• [JIT] Support x % {pow2_cns} transformation for ulong types #79676 — unsigned mod support was extended the same way before

On XARCH, fgMorphSmpOp expands a % b into a - (a / b) * b during morph so that CSE can recognize a shared divisor between a division and a modulo with the same operands. This expansion was gated on tree->OperIs(GT_MOD), so unsigned modulo (GT_UMOD) with a non-power-of-2 constant divisor was never expanded in morph — it was
left for lowering instead. Since CSE runs before lowering, patterns like:

uint arrIndex = index / 3;
uint bitIndex = index % 3;

produced two div instructions instead of one.

fgMorphModToSubMulDiv already handles GT_UMOD correctly (it switches the operator to GT_UDIV and calls CheckDivideByConstOptimized), so the fix is to include GT_UMOD in the condition. Power-of-2 unsigned divisors are unaffected — they are already handled earlier by fgMorphUModToAndSub.

Note: ulong UMOD on XARCH is still blocked by a separate early return that predates this fix and is not addressed here.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates XARCH JIT morphing to expand unsigned modulo (GT_UMOD) by a constant divisor into a - (a / b) * b earlier (during morph) so CSE can reuse a shared division when both / and % appear with the same operands.

Changes:

• Extend the morph-time mod expansion gate from GT_MOD to GT_MOD and GT_UMOD for non-power-of-2 constant divisors.
• Update the surrounding comment to reflect unsigned modulo handling and the CSE motivation.

[EgorBo]

I am not convinced from the diffs it's a good change from size & perfscore unless there is a strong benchmark justification, so let's keep the existing code as is

[BoyBaykiller]

I think this change is a step in the right direction and we shouldn't easily dismiss it.
I took a look at the diffs locally, here is what I found.

1. We have a bunch of size+perf regressions when running without optimizations. This makes sense because the reason we expand, CSE, doesn't run when opts are disabled. Neither does magic division. Also, all the extra locals end up on the stack. The fix is to simply not expand when opts are disabled.

2. We see size regressions but perfscore improvements that look like:
   .
   This is a good thing, in the sense that our optimization that turns imuls into lea/add/shift-sequence now kicks in more.
   The real question is, why does this XARCH specific opt which obfuscates IR doesn't run in lower. Anyway this explains many of the size regressions

3. This is rare, but next up we have added bounds checks. Reason is point 2, so moving imul opts to lower would fix it. Although it could also be interpreted as an opportunity to improve range check.

4. Lastly, I see some of this which should be trivial to fix:

[EgorBo]

I really prefer not to do that this way, expanding a % b to a - (a / b) * b even for b being constant implies we introduce a STORE_LCL_VAR for a here making the tree non-hoistable. I think we should also disable this opt for arm64 and either just do that in lower or have some smart phase that will expand it if it sees that it will enable CSE opportunities

[JulieLeeMSFT]

I really prefer not to do that this way, expanding a % b to a - (a / b) * b even for b being constant implies we introduce a STORE_LCL_VAR for a here making the tree non-hoistable. I think we should also disable this opt for arm64 and either just do that in lower or have some smart phase that will expand it if it sees that it will enable CSE opportunities

Closing this PR because we prefer other approach to enable CSE.