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

src/mono/mono/mini/intrinsics.c

@@ -295,10 +295,10 @@ llvm_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
 		// corresponding APIs on Math/MathF -- e.g. there is no `Math.MinNumber` --
 		// so the existing Math/MathF block above doesn't catch them.
 		//
-		// * MinNumber / MaxNumber: IEEE 754-2008 minNum / maxNum (NaN-suppressing).
-		//   Lower to llvm.minnum / llvm.maxnum, which on AArch64 maps to a single
-		//   fminnm / fmaxnm instruction; matches the BCL spec "if either is NaN
-		//   return the non-NaN; if both are NaN return NaN".
+		// * MinNumber / MaxNumber: IEEE 754-2019 minimumNumber / maximumNumber
+		//   (NaN-suppressing and sign-of-zero aware, treating -0 as less than +0).
+		//   Lowered in mini-llvm.c by composing llvm.minimum / llvm.maximum with an
+		//   explicit NaN fixup; see the OP_FMINNUM case there and llvm-intrinsics.h.
 		// * Abs: BCL forwarder to MathF.Abs / Math.Abs. Today this usually inlines
 		//   into the Math/MathF recognition above, but adding direct recognition
 		//   keeps the lowering working even if the JIT inliner declines.

src/mono/mono/mini/llvm-intrinsics.h

@@ -98,18 +98,11 @@ INTRINS_OVR(TRUNCF, trunc, Generic, LLVMFloatType ())
 INTRINS_OVR(COPYSIGN, copysign, Generic, LLVMDoubleType ())
 INTRINS_OVR(COPYSIGNF, copysign, Generic, LLVMFloatType ())
 	/*
-	 * IEEE 754-2008 minNum/maxNum (NaN-suppressing). When exactly one operand
-	 * is NaN they return the other; when both are NaN they return NaN. This is
-	 * what `float.MinNumber` / `double.MinNumber` (and the Max variants,
-	 * surfaced via INumber<TSelf> on the primitive Single/Double/Half types)
-	 * are documented to do, and on AArch64 these lower to single fminnm/fmaxnm
-	 * instructions. Use llvm.minimum/maximum (see above) for the NaN-propagating
-	 * Math.Min/Math.Max instead.
+	 * `float.MinNumber` / `double.MinNumber` (and the Max variants) are lowered in
+	 * mini-llvm.c by composing llvm.minimum/maximum (above) with an explicit NaN
+	 * fixup; see the OP_FMINNUM case there for why we don't use llvm.minnum/maxnum
+	 * or llvm.minimumnum/maximumnum directly.
 	 */
-INTRINS_OVR(MINNUM, minnum, Generic, LLVMDoubleType ())
-INTRINS_OVR(MINNUMF, minnum, Generic, LLVMFloatType ())
-INTRINS_OVR(MAXNUM, maxnum, Generic, LLVMDoubleType ())
-INTRINS_OVR(MAXNUMF, maxnum, Generic, LLVMFloatType ())
 INTRINS_OVR(EXPECT_I8, expect, Generic, LLVMInt8Type ())
 INTRINS_OVR(EXPECT_I1, expect, Generic, LLVMInt1Type ())
 INTRINS_OVR(CTPOP_I32, ctpop, Generic, LLVMInt32Type ())

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

@@ -7627,24 +7627,33 @@ MONO_RESTORE_WARNING
 		case OP_RMINNUM:
 		case OP_RMAXNUM: {
 			/*
-			 * IEEE 754-2008 minNum/maxNum (NaN-suppressing). Maps directly to
-			 * llvm.minnum/maxnum, which is what `float.MinNumber` /
+			 * IEEE 754-2019 minimumNumber/maximumNumber (NaN-suppressing and
+			 * sign-of-zero aware), as specified by `float.MinNumber` /
 			 * `double.MinNumber` (and the Max variants, surfaced via
-			 * INumber<TSelf> on the primitive Single/Double/Half types) specify.
-			 * On AArch64 this lowers to a single fminnm/fmaxnm instruction.
+			 * INumber<TSelf> on the primitive Single/Double/Half types).
+			 *
+			 * 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
+			 * backend in the LLVM version we build against. When exactly one operand
+			 * is NaN we return the other; when both are NaN the NaN flows through.
 			 */
 			gboolean is_r4 = ins->opcode == OP_RMINNUM || ins->opcode == OP_RMAXNUM;
+			gboolean is_max = ins->opcode == OP_FMAXNUM || ins->opcode == OP_RMAXNUM;
 			LLVMTypeRef t = is_r4 ? LLVMFloatType () : LLVMDoubleType ();
-			LLVMValueRef args [2] = { convert (ctx, lhs, t), convert (ctx, rhs, t) };
-			IntrinsicId iid;
-			switch (ins->opcode) {
-			case OP_FMAXNUM: iid = INTRINS_MAXNUM; break;
-			case OP_FMINNUM: iid = INTRINS_MINNUM; break;
-			case OP_RMAXNUM: iid = INTRINS_MAXNUMF; break;
-			case OP_RMINNUM: iid = INTRINS_MINNUMF; break;
-			default: g_assert_not_reached (); break;
-			}
-			values [ins->dreg] = call_intrins (ctx, iid, args, dname);
+			LLVMValueRef l = convert (ctx, lhs, t);
+			LLVMValueRef r = convert (ctx, rhs, t);
+			LLVMValueRef args [2] = { l, r };
+			IntrinsicId iid = is_max ? (is_r4 ? INTRINS_MAXIMUMF : INTRINS_MAXIMUM)
+			                         : (is_r4 ? INTRINS_MINIMUMF : INTRINS_MINIMUM);
+			LLVMValueRef result = call_intrins (ctx, iid, args, "");
+			LLVMValueRef l_nan = LLVMBuildFCmp (builder, LLVMRealUNO, l, l, "");
+			LLVMValueRef r_nan = LLVMBuildFCmp (builder, LLVMRealUNO, r, r, "");
+			result = LLVMBuildSelect (builder, r_nan, l, result, "");
+			result = LLVMBuildSelect (builder, l_nan, r, result, dname);
+			values [ins->dreg] = result;
 			break;
 		}
 

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

@@ -12,7 +12,6 @@ public static class Runtime_130831
     // Compare the exact bit pattern: Double/Single.Equals treat -0.0 and +0.0 as equal,
     // so a plain Assert.Equal would not observe a wrong-signed-zero result.
     [Fact]
-    [SkipOnMono("https://github.com/dotnet/runtime/issues/131130", TestPlatforms.Any)]
     public static void TestEntryPoint()
     {
         Assert.Equal(BitConverter.DoubleToInt64Bits(-0.0), BitConverter.DoubleToInt64Bits(MinNegZeroConst(+0.0)));