[mono][llvm] Use llvm.minimum/maximum for scalar Math.Min/Max float ops (#129593)

Author: lewing

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Numerics/BFloat16Tests.cs

@@ -538,6 +538,7 @@ public static IEnumerable<object[]> ExplicitConversion_FromSingle_TestData()
 
         [MemberData(nameof(ExplicitConversion_FromSingle_TestData))]
         [Theory]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/103347", TestPlatforms.Browser)]
         public static void ExplicitConversion_FromSingle(float f, BFloat16 expected) // Check the underlying bits for verifying NaNs
         {
             BFloat16 b16 = (BFloat16)f;
@@ -623,6 +624,7 @@ public static IEnumerable<object[]> ExplicitConversion_FromDouble_TestData()
 
         [MemberData(nameof(ExplicitConversion_FromDouble_TestData))]
         [Theory]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/103347", TestPlatforms.Browser)]
         public static void ExplicitConversion_FromDouble(double d, BFloat16 expected) // Check the underlying bits for verifying NaNs
         {
             BFloat16 b16 = (BFloat16)d;

src/mono/mono/mini/intrinsics.c

@@ -199,10 +199,12 @@ llvm_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
 		}
 		// (double, double)
 		if (fsig->param_count == 2 && fsig->params [0]->type == MONO_TYPE_R8 && fsig->params [1]->type == MONO_TYPE_R8) {
-			// Max and Min can only be optimized in fast math mode
-			if (!strcmp (cmethod->name, "Max") && mono_use_fast_math) {
+			// Max/Min use llvm.maximum/minimum (NaN-propagating); see mini-llvm.c.
+			// Matches IEEE 754-2019 minimum/maximum which is what Math.Min/Math.Max
+			// (and the MathF.Min/MathF.Max forwarders) document.
+			if (!strcmp (cmethod->name, "Max")) {
 				opcode = OP_FMAX;
-			} else if (!strcmp (cmethod->name, "Min") && mono_use_fast_math) {
+			} else if (!strcmp (cmethod->name, "Min")) {
 				opcode = OP_FMIN;
 			} else if (!strcmp (cmethod->name, "Pow")) {
 				opcode = OP_FPOW;
@@ -226,9 +228,9 @@ llvm_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
 		}
 		// (float, float)
 		if (fsig->param_count == 2 && fsig->params [0]->type == MONO_TYPE_R4 && fsig->params [1]->type == MONO_TYPE_R4) {
-			if (!strcmp (cmethod->name, "Max") && mono_use_fast_math) {
+			if (!strcmp (cmethod->name, "Max")) {
 				opcode = OP_RMAX;
-			} else if (!strcmp (cmethod->name, "Min") && mono_use_fast_math) {
+			} else if (!strcmp (cmethod->name, "Min")) {
 				opcode = OP_RMIN;
 			} else if (!strcmp (cmethod->name, "Pow")) {
 				opcode = OP_RPOW;

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

@@ -64,6 +64,19 @@ INTRINS_OVR(CEIL, ceil, Generic, LLVMDoubleType ())
 INTRINS_OVR(CEILF, ceil, Generic, LLVMFloatType ())
 INTRINS_OVR(FMA, fma, Generic, LLVMDoubleType ())
 INTRINS_OVR(FMAF, fma, Generic, LLVMFloatType ())
+	/*
+	 * IEEE 754-2019 minimum/maximum (NaN-propagating). Used for scalar OP_FMIN/OP_FMAX/
+	 * OP_RMIN/OP_RMAX. We avoid the older fcmp+select lowering because (a) it has
+	 * asymmetric NaN semantics (Min(NaN,x)=NaN but Min(x,NaN)=x), violating
+	 * Math.Min/Math.Max (and the MathF.Min/MathF.Max forwarders) spec, and (b) on
+	 * AArch64 the backend folds it to fminnm/fmaxnm (IEEE 754-2008 minNum/maxNum),
+	 * which suppresses NaN entirely and miscompiles the System.Half software
+	 * conversion path under LLVM 23.
+	 */
+INTRINS_OVR(MINIMUM, minimum, Generic, LLVMDoubleType ())
+INTRINS_OVR(MINIMUMF, minimum, Generic, LLVMFloatType ())
+INTRINS_OVR(MAXIMUM, maximum, Generic, LLVMDoubleType ())
+INTRINS_OVR(MAXIMUMF, maximum, Generic, LLVMFloatType ())
 	/* This isn't an intrinsic, instead llvm seems to special case it by name */
 INTRINS_OVR(FABS, fabs, Generic, LLVMDoubleType ())
 INTRINS_OVR(ABSF, fabs, Generic, LLVMFloatType ())

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

@@ -7531,11 +7531,7 @@ MONO_RESTORE_WARNING
 		case OP_IMIN_UN:
 		case OP_LMIN_UN:
 		case OP_IMAX_UN:
-		case OP_LMAX_UN:
-		case OP_FMIN:
-		case OP_FMAX:
-		case OP_RMIN:
-		case OP_RMAX: {
+		case OP_LMAX_UN: {
 			LLVMValueRef v;
 
 			lhs = convert (ctx, lhs, regtype_to_llvm_type (spec [MONO_INST_DEST]));
@@ -7558,14 +7554,6 @@ MONO_RESTORE_WARNING
 			case OP_LMAX_UN:
 				v = LLVMBuildICmp (builder, LLVMIntUGE, lhs, rhs, "");
 				break;
-			case OP_FMAX:
-			case OP_RMAX:
-				v = LLVMBuildFCmp (builder, LLVMRealUGE, lhs, rhs, "");
-				break;
-			case OP_FMIN:
-			case OP_RMIN:
-				v = LLVMBuildFCmp (builder, LLVMRealULE, lhs, rhs, "");
-				break;
 			default:
 				g_assert_not_reached ();
 				break;
@@ -7574,6 +7562,33 @@ MONO_RESTORE_WARNING
 			break;
 		}
 
+		case OP_FMIN:
+		case OP_FMAX:
+		case OP_RMIN:
+		case OP_RMAX: {
+			/*
+			 * Use llvm.minimum/maximum (IEEE 754-2019, NaN-propagating) so the
+			 * Math.Min/Math.Max semantics ("if either argument is NaN, NaN is
+			 * returned"), as forwarded by MathF.Min/MathF.Max, are honored
+			 * symmetrically. The old fcmp+select lowering was both asymmetric for
+			 * NaN and got folded into AArch64 fminnm/fmaxnm by the backend, which
+			 * discards NaN entirely. See llvm-intrinsics.h.
+			 */
+			gboolean is_r4 = ins->opcode == OP_RMIN || ins->opcode == OP_RMAX;
+			LLVMTypeRef t = is_r4 ? LLVMFloatType () : LLVMDoubleType ();
+			LLVMValueRef args [2] = { convert (ctx, lhs, t), convert (ctx, rhs, t) };
+			IntrinsicId iid;
+			switch (ins->opcode) {
+			case OP_FMAX: iid = INTRINS_MAXIMUM; break;
+			case OP_FMIN: iid = INTRINS_MINIMUM; break;
+			case OP_RMAX: iid = INTRINS_MAXIMUMF; break;
+			case OP_RMIN: iid = INTRINS_MINIMUMF; break;
+			default: g_assert_not_reached (); break;
+			}
+			values [ins->dreg] = call_intrins (ctx, iid, args, dname);
+			break;
+		}
+
 /*
  * See the ARM64 comment in mono/utils/atomic.h for an explanation of why this
  * hack is necessary (for now).