Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero - #131172

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero
Jul 23, 2026
Merged

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero#131172
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-mono-min-max-sign-of-zero

Conversation

@tannergooding

Copy link
Copy Markdown
Member

Fixes#131130.

double.MinNumber/MaxNumber (and the float/Half variants) implement IEEE 754-2019 minimumNumber/maximumNumber, which are NaN-suppressing and treat -0 as less than +0. The Mono LLVM scalar lowering used llvm.minnum/maxnum (IEEE 754-2008 minNum/maxNum), which leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86, so double.MinNumber(+0.0, -0.0) returned +0.0 instead of -0.0 on the LLVM AOT configuration.

Lower to llvm.minimumnum/maximumnum instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 minimumNumber/maximumNumber operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single fminnm/fmaxnm. Min/Max already use llvm.minimum/maximum and are correct, so they are unchanged.

I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: minnum(+0, -0) returns +0 (wrong), while minimumnum(+0, -0) returns -0 (correct) and still suppresses NaN.


Un-skips the Runtime_130831 regression test (added in #130832) on Mono, which covers the Min/MinNumber signed-zero cases directly and runs on the AllSubsets_Mono_LLVMAot_RuntimeTests leg.

Note

This change was authored with the assistance of GitHub Copilot.

… zero
double.MinNumber/MaxNumber (IEEE 754-2019 minimumNumber/maximumNumber) treat
-0 as less than +0, but the scalar lowering used llvm.minnum/maxnum, which
leave the sign of zero unspecified -- e.g. minnum(+0, -0) returns +0 on x86.
Lower to llvm.minimumnum/maximumnum instead, which are NaN-suppressing and
sign-of-zero aware. Min/Max already use llvm.minimum/maximum and are correct.
Un-skips Runtime_130831 on Mono now that the divergence is fixed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Mono’s LLVM backend to correctly implement IEEE 754-2019 minimumNumber / maximumNumber semantics for MinNumber / MaxNumber, specifically preserving the sign of zero (e.g., min(+0, -0) => -0) on LLVM AOT. It also re-enables a previously skipped regression test on Mono now that the behavior should match the BCL contract.

Changes:

  • Switch Mono LLVM scalar lowering of MinNumber/MaxNumber from llvm.minnum/llvm.maxnum to llvm.minimumnum/llvm.maximumnum.
  • Update the intrinsic ID list to use the new LLVM intrinsic names.
  • Unskip Runtime_130831 on Mono to validate signed-zero behavior.
Show a summary per file
FileDescription
src/mono/mono/mini/mini-llvm.cRoutes OP_*MINNUM/OP_*MAXNUM to the new MINIMUMNUM/MAXIMUMNUM intrinsics during LLVM emission.
src/mono/mono/mini/llvm-intrinsics.hReplaces MINNUM/MAXNUM intrinsic definitions with MINIMUMNUM/MAXIMUMNUM to match the intended semantics.
src/mono/mono/mini/intrinsics.cUpdates commentary/documentation for MinNumber/MaxNumber lowering to reflect the new LLVM intrinsics and semantics.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.csRemoves [SkipOnMono] so the signed-zero regression test runs on Mono again.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:22

  • The test now runs on Mono and validates the signed-zero behavior for Min/MinNumber, but this PR also changes the LLVM lowering for MaxNumber. It would be good to add Max/MaxNumber signed-zero assertions here as well so the new MAXIMUMNUM lowering is covered and a future regression is caught.
 [Fact]
public static void TestEntryPoint()
{
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@tannergooding
tannergooding enabled auto-merge (squash) July 21, 2026 22:55
…scompiled minimumnum/maximumnum
llvm.minimumnum/maximumnum have the right IEEE 754-2019 semantics but are
miscompiled by the x86 backend in the LLVM version Mono builds against,
regressing Runtime_98068.TestMinNumber on the Mono LLVM AOT leg. Lower
MinNumber/MaxNumber by composing llvm.minimum/maximum (sign-of-zero aware,
already used for Math.Min/Max) with an explicit NaN fixup instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 22, 2026 14:32
@tannergooding

Copy link
Copy Markdown
MemberAuthor

The AllSubsets_Mono_LLVMAot_RuntimeTests legs failed on Runtime_98068.TestMinNumber() (double.MinNumber(NaN, +1.0) returned garbage). The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against. Confirmed this was the regression (baseline main passes the same leg).

Fixed by composing MinNumber/MaxNumber from llvm.minimum/maximum (sign-of-zero aware, already used for Math.Min/Max) plus an explicit NaN fixup, rather than lowering to the broken intrinsic. This keeps the sign-of-zero fix while avoiding the miscompile. Verified the composed IR bitwise across the NaN / signed-zero edge cases; end-to-end validation is on the re-run of the Mono LLVM AOT leg here.

Note

This comment was drafted by Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs:21

  • This test now runs on Mono LLVM AOT, but it only validates the Min/MinNumber signed-zero behavior. The PR also changes the MaxNumber lowering (OP_*MAXNUM), so adding symmetric assertions for Max/MaxNumber would help prevent regressions in that path as well.
 Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));
Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNumberZeroConst(-0.0)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNegZeroConst(+0.0f)));
Assert.Equal(BitConverter.SingleToInt32Bits(-0.0f), BitConverter.SingleToInt32Bits(MinNumberZeroConst(-0.0f)));

src/mono/mono/mini/mini-llvm.c:7639

  • The PR description says OP_*MINNUM/OP_*MAXNUM are lowered to llvm.minimumnum/maximumnum, but this implementation instead composes llvm.minimum/maximum with explicit NaN fixups and even states minimumnum/maximumnum are miscompiled on x86. Please reconcile (either switch to the new intrinsics as described, or update the PR description / add an LLVM issue reference + clarify the expected codegen impact, especially on AArch64).
 * We compose this from llvm.minimum/maximum (sign-of-zero aware but
* NaN-propagating) plus an explicit NaN fixup, rather than lowering
* directly to an intrinsic: llvm.minnum/maxnum leave the sign of zero
* unspecified (minnum(+0, -0) may return +0 on x86, see dotnet/runtime
* #131130) and llvm.minimumnum/maximumnum are miscompiled by the x86
  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment threadsrc/mono/mono/mini/mini-llvm.c
@akoeplinger

Copy link
Copy Markdown
Member

The llvm.minimumnum/maximumnum intrinsics from the first commit have the right IEEE 754-2019 semantics, but are miscompiled by the x86 backend in the LLVM version Mono builds against.

should we file an llvm bug for this?

@tannergooding

Copy link
Copy Markdown
MemberAuthor

should we file an llvm bug for this?

Was already working on that. I've logged a few issues against Clang/LLVM in the past few days 😅

@tannergooding

Copy link
Copy Markdown
MemberAuthor

@akoeplinger, looks like this is already fixed upstream

Copilot reports back, and then I verified after:

LLVM minimumnum miscompile — pin & fix investigation

What Mono is pinned to

  • Package:23.1.0-alpha.1.26357.1 from dotnet/llvm-project (eng/Version.Details.xml).
  • Snapshot commit:04bf14b2 (2026-07-07) — an arcade dep-update only; it did not advance LLVM sources.
  • Effective LLVM sources: upstream llvm/llvm-projectmain @ a72232481fe7 (2026-06-13) — the fork's last merge-upstream-main (c9811b58) — plus a handful of unrelated Mono-local patches.

The bug is upstream, not a Mono patch — proven

The dnceng LLVM tree = upstream base + Mono patches. I compared git blob SHAs of all 9 files implementing float min/max lowering across the two forks; every one is byte-identical to upstreama72232481fe7:

X86ISelLowering.cpp, SelectionDAG/{LegalizeDAG, LegalizeFloatTypes, LegalizeVectorTypes, LegalizeVectorOps, SelectionDAGBuilder, DAGCombiner}.cpp, TargetLoweringBase.cpp, Analysis/ConstantFolding.cpp — all SAME.

The Mono-local patches (mono.this metadata, MonoException.cpp, libatomic, LLVMConstInt truncation) don't touch min/max lowering.

Regression window

Revisionminimumnum(NaN, 1.0)
LLVM 22.1.0 release✅ correct
upstream main @ 2026-06-13 (a72232481fe7, the pin)-1.0 (per Mono CI)
current trunk✅ correct

Trunk-only regression: introduced after the 22.x branch, already fixed on trunk. The dnceng snapshot landed inside the window. Bug is asymmetricminimumnum broken, maximumnum fine.

Supposed fix (not yet in the fork)

dotnet/main last merged upstream at c9811b58 (2026-06-13), so the fork lacks these later X86 changes:

  • 961ab7a3 (2026-07-08) — [SDAG][X86] Uplift pseudo fmin/fmax from X86 (#188489) — leading suspect; removes X86's own 177-line pseudo fmin/fmax lowering (the exact path the pin still uses) and re-homes it in SDAG.
  • 8a3b7152 (2026-07-01) — [X86] Fold FP UNORD/ORD compare against known-non-NaN to self-compare (#206943) — touches the cmpordsd NaN logic; lower confidence.

Caveat: couldn't build llc at a72232481fe7 to execute + git-bisect the exact fix, so #188489 is the leading suspect by code path, not proven. The "not a Mono patch" conclusion is proven via blob comparison.

Fix path

Fresh merge-upstream-main into dotnet/llvm-project past 2026-07-08 (pulls in #188489), then bump the snapshot in eng/Version.Details.xml.

ItemValue
Mono LLVM pin23.1.0-alpha.1.26357.1 → dotnet/llvm-project 04bf14b2
Effective upstream basea72232481fe7 (main @ 2026-06-13)
Suspected upstream fix#188489 961ab7a3 (2026-07-08)

@akoeplinger

Copy link
Copy Markdown
Member

ok. I see commit 961ab7a3 made it into the 23.x release branch which we're planning to sync to in a bit.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

made it into the 23.x release branch which we're planning to sync to in a bit.

@akoeplinger, if you can ping me when this happens I can either sign-off on or remove the extra workaround here

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 24, 2026
@akoeplinger

Copy link
Copy Markdown
Member

@tannergooding#132272 brought in the LLVM bump

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mono: Min/MinNumber lowering does not preserve the sign of zero

4 participants

@tannergooding@akoeplinger@lewing