APFloat: Fix signed zero handling in minnum/maxnum #83376

arsenm · 2024-02-29T04:55:35Z

Follow the 2019 rules and order -0 as less than +0 and +0 as greater than -0. As currently defined this isn't required for the intrinsics, but is a better QoI.

This will avoid the workaround in libc added by #83158

Follow the 2019 rules and order -0 as less than +0 and +0 as greater than -0. As currently defined this isn't required for the intrinsics, but is a better QoI. This will avoid the workaround in libc added by llvm#83158

llvmbot · 2024-02-29T04:56:04Z

@llvm/pr-subscribers-llvm-adt
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

Follow the 2019 rules and order -0 as less than +0 and +0 as greater than -0. As currently defined this isn't required for the intrinsics, but is a better QoI.

This will avoid the workaround in libc added by #83158

Full diff: https://github.com/llvm/llvm-project/pull/83376.diff

7 Files Affected:

(modified) llvm/include/llvm/ADT/APFloat.h (+4)
(modified) llvm/test/CodeGen/AMDGPU/fmaxnum.ll (+1-1)
(modified) llvm/test/CodeGen/AMDGPU/fminnum.ll (+1-1)
(modified) llvm/test/Transforms/InstCombine/maxnum.ll (+1-1)
(modified) llvm/test/Transforms/InstCombine/minnum.ll (+2-2)
(modified) llvm/test/Transforms/InstSimplify/ConstProp/min-max.ll (+66-2)
(modified) llvm/unittests/ADT/APFloatTest.cpp (+10)

diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index 8c247bbcec90a2..598d00f592f8c0 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -1397,6 +1397,8 @@ inline APFloat minnum(const APFloat &A, const APFloat &B) {
     return B;
   if (B.isNaN())
     return A;
+  if (A.isZero() && B.isZero() && (A.isNegative() != B.isNegative()))
+    return A.isNegative() ? A : B;
   return B < A ? B : A;
 }
 
@@ -1408,6 +1410,8 @@ inline APFloat maxnum(const APFloat &A, const APFloat &B) {
     return B;
   if (B.isNaN())
     return A;
+  if (A.isZero() && B.isZero() && (A.isNegative() != B.isNegative()))
+    return A.isNegative() ? B : A;
   return A < B ? B : A;
 }
 
diff --git a/llvm/test/CodeGen/AMDGPU/fmaxnum.ll b/llvm/test/CodeGen/AMDGPU/fmaxnum.ll
index 09898f1442fb82..38640a18b5aee6 100644
--- a/llvm/test/CodeGen/AMDGPU/fmaxnum.ll
+++ b/llvm/test/CodeGen/AMDGPU/fmaxnum.ll
@@ -152,7 +152,7 @@ define amdgpu_kernel void @constant_fold_fmax_f32_p0_n0(ptr addrspace(1) %out) #
 
 ; GCN-LABEL: {{^}}constant_fold_fmax_f32_n0_p0:
 ; GCN-NOT: v_max_f32_e32
-; GCN: v_bfrev_b32_e32 [[REG:v[0-9]+]], 1{{$}}
+; GCN: v_mov_b32_e32 [[REG:v[0-9]+]], 0{{$}}
 ; GCN: buffer_store_dword [[REG]]
 define amdgpu_kernel void @constant_fold_fmax_f32_n0_p0(ptr addrspace(1) %out) #0 {
   %val = call float @llvm.maxnum.f32(float -0.0, float 0.0)
diff --git a/llvm/test/CodeGen/AMDGPU/fminnum.ll b/llvm/test/CodeGen/AMDGPU/fminnum.ll
index 844d26a6225b40..65b311845a6b77 100644
--- a/llvm/test/CodeGen/AMDGPU/fminnum.ll
+++ b/llvm/test/CodeGen/AMDGPU/fminnum.ll
@@ -150,7 +150,7 @@ define amdgpu_kernel void @constant_fold_fmin_f32_p0_p0(ptr addrspace(1) %out) #
 
 ; GCN-LABEL: {{^}}constant_fold_fmin_f32_p0_n0:
 ; GCN-NOT: v_min_f32_e32
-; GCN: v_mov_b32_e32 [[REG:v[0-9]+]], 0
+; GCN: v_bfrev_b32_e32 [[REG:v[0-9]+]], 1{{$}}
 ; GCN: buffer_store_dword [[REG]]
 define amdgpu_kernel void @constant_fold_fmin_f32_p0_n0(ptr addrspace(1) %out) #0 {
   %val = call float @llvm.minnum.f32(float 0.0, float -0.0)
diff --git a/llvm/test/Transforms/InstCombine/maxnum.ll b/llvm/test/Transforms/InstCombine/maxnum.ll
index 87288b18cbcd9f..e140a5b405ea84 100644
--- a/llvm/test/Transforms/InstCombine/maxnum.ll
+++ b/llvm/test/Transforms/InstCombine/maxnum.ll
@@ -66,7 +66,7 @@ define float @constant_fold_maxnum_f32_p0_n0() {
 
 define float @constant_fold_maxnum_f32_n0_p0() {
 ; CHECK-LABEL: @constant_fold_maxnum_f32_n0_p0(
-; CHECK-NEXT:    ret float -0.000000e+00
+; CHECK-NEXT:    ret float 0.000000e+00
 ;
   %x = call float @llvm.maxnum.f32(float -0.0, float 0.0)
   ret float %x
diff --git a/llvm/test/Transforms/InstCombine/minnum.ll b/llvm/test/Transforms/InstCombine/minnum.ll
index 8050f075595272..cc6171b9d8e6cb 100644
--- a/llvm/test/Transforms/InstCombine/minnum.ll
+++ b/llvm/test/Transforms/InstCombine/minnum.ll
@@ -60,7 +60,7 @@ define float @constant_fold_minnum_f32_p0_p0() {
 
 define float @constant_fold_minnum_f32_p0_n0() {
 ; CHECK-LABEL: @constant_fold_minnum_f32_p0_n0(
-; CHECK-NEXT:    ret float 0.000000e+00
+; CHECK-NEXT:    ret float -0.000000e+00
 ;
   %x = call float @llvm.minnum.f32(float 0.0, float -0.0)
   ret float %x
@@ -199,7 +199,7 @@ define float @minnum_f32_1_minnum_p0_val_fmf3(float %x) {
 
 define float @minnum_f32_p0_minnum_val_n0(float %x) {
 ; CHECK-LABEL: @minnum_f32_p0_minnum_val_n0(
-; CHECK-NEXT:    [[Z:%.*]] = call float @llvm.minnum.f32(float [[X:%.*]], float 0.000000e+00)
+; CHECK-NEXT:    [[Z:%.*]] = call float @llvm.minnum.f32(float [[X:%.*]], float -0.000000e+00)
 ; CHECK-NEXT:    ret float [[Z]]
 ;
   %y = call float @llvm.minnum.f32(float %x, float -0.0)
diff --git a/llvm/test/Transforms/InstSimplify/ConstProp/min-max.ll b/llvm/test/Transforms/InstSimplify/ConstProp/min-max.ll
index a5f5d4e12ed842..9120649eb5c4f1 100644
--- a/llvm/test/Transforms/InstSimplify/ConstProp/min-max.ll
+++ b/llvm/test/Transforms/InstSimplify/ConstProp/min-max.ll
@@ -49,6 +49,38 @@ define float @minnum_float() {
   ret float %1
 }
 
+define float @minnum_float_p0_n0() {
+; CHECK-LABEL: @minnum_float_p0_n0(
+; CHECK-NEXT:    ret float -0.000000e+00
+;
+  %min = call float @llvm.minnum.f32(float 0.0, float -0.0)
+  ret float %min
+}
+
+define float @minnum_float_n0_p0() {
+; CHECK-LABEL: @minnum_float_n0_p0(
+; CHECK-NEXT:    ret float -0.000000e+00
+;
+  %min = call float @llvm.minnum.f32(float -0.0, float 0.0)
+  ret float %min
+}
+
+define float @minnum_float_p0_qnan() {
+; CHECK-LABEL: @minnum_float_p0_qnan(
+; CHECK-NEXT:    ret float 0.000000e+00
+;
+  %min = call float @llvm.minnum.f32(float 0.0, float 0x7FF8000000000000)
+  ret float %min
+}
+
+define float @minnum_float_qnan_p0() {
+; CHECK-LABEL: @minnum_float_qnan_p0(
+; CHECK-NEXT:    ret float 0.000000e+00
+;
+  %min = call float @llvm.minnum.f32(float 0x7FF8000000000000, float 0.0)
+  ret float %min
+}
+
 define bfloat @minnum_bfloat() {
 ; CHECK-LABEL: @minnum_bfloat(
 ; CHECK-NEXT:    ret bfloat 0xR40A0
@@ -95,7 +127,7 @@ define <4 x half> @minnum_half_vec() {
 
 define <4 x float> @minnum_float_zeros_vec() {
 ; CHECK-LABEL: @minnum_float_zeros_vec(
-; CHECK-NEXT:    ret <4 x float> <float 0.000000e+00, float -0.000000e+00, float 0.000000e+00, float -0.000000e+00>
+; CHECK-NEXT:    ret <4 x float> <float 0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00>
 ;
   %1 = call <4 x float> @llvm.minnum.v4f32(<4 x float> <float 0.0, float -0.0, float 0.0, float -0.0>, <4 x float> <float 0.0, float 0.0, float -0.0, float -0.0>)
   ret <4 x float> %1
@@ -109,6 +141,38 @@ define float @maxnum_float() {
   ret float %1
 }
 
+define float @maxnum_float_p0_n0() {
+; CHECK-LABEL: @maxnum_float_p0_n0(
+; CHECK-NEXT:    ret float 0.000000e+00
+;
+  %max = call float @llvm.maxnum.f32(float 0.0, float -0.0)
+  ret float %max
+}
+
+define float @maxnum_float_n0_p0() {
+; CHECK-LABEL: @maxnum_float_n0_p0(
+; CHECK-NEXT:    ret float 0.000000e+00
+;
+  %max = call float @llvm.maxnum.f32(float -0.0, float 0.0)
+  ret float %max
+}
+
+define float @maxnum_float_p0_qnan() {
+; CHECK-LABEL: @maxnum_float_p0_qnan(
+; CHECK-NEXT:    ret float 0.000000e+00
+;
+  %max = call float @llvm.maxnum.f32(float 0.0, float 0x7FF8000000000000)
+  ret float %max
+}
+
+define float @maxnum_float_qnan_p0() {
+; CHECK-LABEL: @maxnum_float_qnan_p0(
+; CHECK-NEXT:    ret float 0.000000e+00
+;
+  %max = call float @llvm.maxnum.f32(float 0x7FF8000000000000, float 0.0)
+  ret float %max
+}
+
 define bfloat @maxnum_bfloat() {
 ; CHECK-LABEL: @maxnum_bfloat(
 ; CHECK-NEXT:    ret bfloat 0xR4228
@@ -155,7 +219,7 @@ define <4 x half> @maxnum_half_vec() {
 
 define <4 x float> @maxnum_float_zeros_vec() {
 ; CHECK-LABEL: @maxnum_float_zeros_vec(
-; CHECK-NEXT:    ret <4 x float> <float 0.000000e+00, float -0.000000e+00, float 0.000000e+00, float -0.000000e+00>
+; CHECK-NEXT:    ret <4 x float> <float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float -0.000000e+00>
 ;
   %1 = call <4 x float> @llvm.maxnum.v4f32(<4 x float> <float 0.0, float -0.0, float 0.0, float -0.0>, <4 x float> <float 0.0, float 0.0, float -0.0, float -0.0>)
   ret <4 x float> %1
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index baf055e503b7e7..6e4dda8351a1b1 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -578,6 +578,11 @@ TEST(APFloatTest, MinNum) {
   EXPECT_EQ(1.0, minnum(f2, f1).convertToDouble());
   EXPECT_EQ(1.0, minnum(f1, nan).convertToDouble());
   EXPECT_EQ(1.0, minnum(nan, f1).convertToDouble());
+
+  APFloat zp(0.0);
+  APFloat zn(-0.0);
+  EXPECT_EQ(-0.0, minnum(zp, zn).convertToDouble());
+  EXPECT_EQ(-0.0, minnum(zn, zp).convertToDouble());
 }
 
 TEST(APFloatTest, MaxNum) {
@@ -589,6 +594,11 @@ TEST(APFloatTest, MaxNum) {
   EXPECT_EQ(2.0, maxnum(f2, f1).convertToDouble());
   EXPECT_EQ(1.0, maxnum(f1, nan).convertToDouble());
   EXPECT_EQ(1.0, maxnum(nan, f1).convertToDouble());
+
+  APFloat zp(0.0);
+  APFloat zn(-0.0);
+  EXPECT_EQ(0.0, maxnum(zp, zn).convertToDouble());
+  EXPECT_EQ(0.0, maxnum(zn, zp).convertToDouble());
 }
 
 TEST(APFloatTest, Minimum) {

llvm/include/llvm/ADT/APFloat.h

jayfoad

LGTM. Could also actually rename the functions to minimumNumber/maximumNumber.

Summary: These hacks can be removed now that llvm#83376 fixed the underlying problem.

…83421) Summary: These hacks can be removed now that #83376 fixed the underlying problem.

APFloat: Fix signed zero handling in minnum/maxnum

29f6732

Follow the 2019 rules and order -0 as less than +0 and +0 as greater than -0. As currently defined this isn't required for the intrinsics, but is a better QoI. This will avoid the workaround in libc added by llvm#83158

arsenm added the floating-point Floating-point math label Feb 29, 2024

arsenm requested review from spavloff, mcinally, jhuber6, andykaylor and jcranmer-intel February 29, 2024 04:55

llvmbot added backend:AMDGPU llvm:transforms llvm:adt labels Feb 29, 2024

lntue reviewed Feb 29, 2024

View reviewed changes

llvm/include/llvm/ADT/APFloat.h Show resolved Hide resolved

arsenm added 2 commits February 29, 2024 15:28

Merge branch 'main' into apfloat-signed-zero-min-max

6031db5

Update IEEE spec reference

dfb5552

jayfoad reviewed Feb 29, 2024

View reviewed changes

llvm/include/llvm/ADT/APFloat.h Outdated Show resolved Hide resolved

Complete signed zero documentation

68300dd

jayfoad approved these changes Feb 29, 2024

View reviewed changes

arsenm merged commit 6cfd343 into llvm:main Feb 29, 2024
3 of 4 checks passed

arsenm deleted the apfloat-signed-zero-min-max branch February 29, 2024 11:21

jhuber6 added a commit to jhuber6/llvm-project that referenced this pull request Feb 29, 2024

[libc] Remove workaround for fmin / fmax after being fixed in LLVM

0e7c539

Summary: These hacks can be removed now that llvm#83376 fixed the underlying problem.

jhuber6 mentioned this pull request Feb 29, 2024

[libc] Remove workaround for fmin / fmax after being fixed in LLVM #83421

Merged

jhuber6 added a commit that referenced this pull request Feb 29, 2024

[libc] Remove workaround for fmin / fmax after being fixed in LLVM (#…

23f3651

…83421) Summary: These hacks can be removed now that #83376 fixed the underlying problem.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APFloat: Fix signed zero handling in minnum/maxnum #83376

APFloat: Fix signed zero handling in minnum/maxnum #83376

arsenm commented Feb 29, 2024

llvmbot commented Feb 29, 2024 •

edited

jayfoad left a comment

APFloat: Fix signed zero handling in minnum/maxnum #83376

APFloat: Fix signed zero handling in minnum/maxnum #83376

Conversation

arsenm commented Feb 29, 2024

llvmbot commented Feb 29, 2024 • edited

jayfoad left a comment

Choose a reason for hiding this comment

llvmbot commented Feb 29, 2024 •

edited