JIT: don't distribute negation over DIV by the type minimum - #133529
Open
EgorBo wants to merge 1 commit into
Open
JIT: don't distribute negation over DIV by the type minimum#133529EgorBo wants to merge 1 commit into
EgorBo wants to merge 1 commit into
Conversation
NEG(DIV(a, C)) => DIV(a, -C) is only valid when -C is exactly representable. For C == INT_MIN/INT64_MIN the negation wraps back to C, silently dropping it. Fixes dotnet#133517 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b9596b9-f3b3-47d1-b1ca-1a8781e56de7
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new MIN check misses TYP_I_IMPL on 32-bit targets, so the transformation still miscompiles -(x / nint.MinValue) and can also hit undefined signed-overflow behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates CoreCLR JIT morphing to avoid distributing NEG over DIV when the divisor’s negation is not exactly representable (the signed type minimum), and adds a JIT regression test to cover int, long, and nint cases.
Changes:
- Update
fgMorphSmpOp’sNEG(DIV(a, C)) => DIV(a, -C)transform to excludeC == MINin addition toC == ±1. - Add a new JitBlue regression test covering
int/long/nintand a few nearby invariants. - Register the new test source in
Regression_ro_2.csproj.
File summaries
| File | Description |
|---|---|
| src/coreclr/jit/morph.cpp | Extends the safety check for distributing negation over division constants. |
| src/tests/JIT/Regression/JitBlue/Runtime_133517/Runtime_133517.cs | Adds coverage for the incorrect MIN-divisor negation-distribution behavior across int/long/nint. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new regression test in the build. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+8099
to
+8106
| int64_t constVal = op1op2->AsIntCon()->IconValue(); | ||
| int64_t minVal = INT64_MIN; | ||
|
|
||
| if (mulOrDiv->TypeIs(TYP_INT)) | ||
| { | ||
| constVal = static_cast<int32_t>(constVal); | ||
| minVal = INT32_MIN; | ||
| } |
This was referenced Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #133517
Morph's
NEG(DIV(a, C)) => DIV(a, -C)is only valid when-Cis exactly representable. ForC == int.MinValue/long.MinValuethe negation wraps back toC, so theNEGwas silently dropped.No SPMI asm diffs.