JIT: ensure magic divisors are always marked DONT_CSE for all targets#129856
Conversation
fgMorphSmpOpOptional was calling CheckDivideByConstOptimized for GT_UDIV/GT_UMOD but not for signed GT_DIV, leaving the constant divisor unmarked. If CSE later rewrites it to a LCL_VAR, magic-divide expansion in LowerSignedDivOrMod bails and falls back to idiv. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates JIT morphing so GT_DIV nodes also call CheckDivideByConstOptimized, matching existing handling for GT_UDIV/GT_UMOD. The intent is to keep eligible constant divisors from being constant-CSE’d into locals so later lowering can still recognize and apply “magic divide” transformations.
Changes:
- In
fgMorphSmpOpOptional, callCheckDivideByConstOptimizedforGT_DIVafter the existingval / 1fast-path.
…isors Lowering's TryLowerConstIntDivOrMod generates magic-divide for any non-pow2 signed divisor with |d| >= 3 and a shift+neg sequence for negative pow2 divisors. Mirror that here so the corresponding DONT_CSE marker is set on the divisor constant. Addresses review feedback on dotnet#129856. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Good catch. Pushed f11b2fa which extends Verified on the original repro (System.TimeOnly.AddTicks already uses positive divisors so unchanged), and a quick standalone test with Note Comment drafted with the assistance of GitHub Copilot. |
|
@EgorBo PTAL future-proofing so we can look into enabling const CSE for more platforms. Should be no diff. |
…#129856) `fgMorphSmpOpOptional` calls `CheckDivideByConstOptimized` for `GT_UDIV`/`GT_UMOD` but the corresponding `case GT_DIV:` arm only checks for `val / 1` and breaks. Signed integer divides with a constant divisor reach lowering without `GTF_DONT_CSE` on the divisor. Magic-divide expansion in `LowerSignedDivOrMod` -> `TryLowerConstIntDivOrMod` bails at `if (!divisor->IsCnsIntOrI()) return false;` and falls back to `idiv` if CSE has rewritten the divisor to a `LCL_VAR`. This is latent on x64 because the default `JitConstCSE` setting disables most integer-constant CSE there, so a full SPMI sweep on x64 shows zero asm diffs. It is observable on ARM64/RISC-V where constant CSE is enabled, and would also surface immediately on x64 if `JitConstCSE` is ever broadened. Signed `GT_MOD` is already covered: `fgMorphModToSubMulDiv` transforms `a % b` into `a - (a / b) * b` and calls `CheckDivideByConstOptimized` on the inner `DIV`. > [!NOTE] > This PR description was drafted with the assistance of GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fgMorphSmpOpOptionalcallsCheckDivideByConstOptimizedforGT_UDIV/GT_UMODbut the correspondingcase GT_DIV:arm only checks forval / 1and breaks. Signed integer divides with a constant divisor reach lowering withoutGTF_DONT_CSEon the divisor.Magic-divide expansion in
LowerSignedDivOrMod->TryLowerConstIntDivOrModbails atif (!divisor->IsCnsIntOrI()) return false;and falls back toidivif CSE has rewritten the divisor to aLCL_VAR.This is latent on x64 because the default
JitConstCSEsetting disables most integer-constant CSE there, so a full SPMI sweep on x64 shows zero asm diffs. It is observable on ARM64/RISC-V where constant CSE is enabled, and would also surface immediately on x64 ifJitConstCSEis ever broadened.Signed
GT_MODis already covered:fgMorphModToSubMulDivtransformsa % bintoa - (a / b) * band callsCheckDivideByConstOptimizedon the innerDIV.Note
This PR description was drafted with the assistance of GitHub Copilot.