diff mbox

DBL_DENORM_MIN should never be 0

Message ID alpine.DEB.2.11.1409102021290.1557@laptop-mg.saclay.inria.fr
State New
Headers show

Commit Message

Marc Glisse Sept. 11, 2014, 5:55 a.m. UTC
On Wed, 10 Sep 2014, Joseph S. Myers wrote:

> On Wed, 10 Sep 2014, Marc Glisse wrote:
>
>> Hello,
>>
>> according to the C++ standard, numeric_limits::denorm_min should return min
>> (not 0) when there are no denormals.
>>
>> Tested with bootstrap+testsuite on x86_64-linux-gnu. I also tested a basic
>> make all-gcc for vax (only target without denormals apparently) and the macro
>> did change as expected.
>>
>> The next step might be to define has_denorm as false in more cases (-mno-ieee
>> on alpha, -ffast-math on x86, etc) but that's a different issue.
>>
>> (this is C++ but I believe Joseph is the floating-point expert, hence the cc)
>
> This is a C issue as well (for C11 *_TRUE_MIN).

Ah, indeed. I am updating the comment. And also simplifying float.h. I 
could instead do the same #ifdef in C++, but since both users of 
__FLT_DENORM_MIN__ want the same thing, it seems better to provide that 
directly.

>> gcc/c-family/
>>
>> 2014-09-10  Marc Glisse  <marc.glisse@inria.fr>
>>
>
> 	PR target/58757
>
>> 	* c-cppbuiltin.c (builtin_define_float_constants): Correct
>> 	__*_DENORM_MIN__ without denormals.
>
> I think there should be some sort of testcase that these values aren't 0.

I don't know what kind of test you have in mind, so I added a runtime 
test. I am just guessing that it probably fails on alpha because of PR 
58757, I can't test. Computing d+d may be even more likely to trigger 
potential issues, if that's the goal.

Passes bootstrap+testsuite on x86_64-linux-gnu.

2014-09-11  Marc Glisse  <marc.glisse@inria.fr>

 	PR target/58757
gcc/c-family/
 	* c-cppbuiltin.c (builtin_define_float_constants): Correct
 	__*_DENORM_MIN__ without denormals.
gcc/
 	* ginclude/float.h (FLT_TRUE_MIN, DBL_TRUE_MIN, LDBL_TRUE_MIN):
 	Directly forward to __*_DENORM_MIN__.
gcc/testsuite/
 	* gcc.dg/c11-true_min-1.c: New testcase.

Comments

Joseph Myers Sept. 11, 2014, 4:50 p.m. UTC | #1
On Thu, 11 Sep 2014, Marc Glisse wrote:

> I don't know what kind of test you have in mind, so I added a runtime test. I
> am just guessing that it probably fails on alpha because of PR 58757, I can't
> test. Computing d+d may be even more likely to trigger potential issues, if
> that's the goal.

Yes, a runtime test.  I don't think there should be an xfail without it 
actually having been tested to fail (and then such an xfail should come 
with a comment referencing the bug filed in Bugzilla).
Marc Glisse Sept. 11, 2014, 6:03 p.m. UTC | #2
On Thu, 11 Sep 2014, Joseph S. Myers wrote:

> On Thu, 11 Sep 2014, Marc Glisse wrote:
>
>> I don't know what kind of test you have in mind, so I added a runtime test. I
>> am just guessing that it probably fails on alpha because of PR 58757, I can't
>> test. Computing d+d may be even more likely to trigger potential issues, if
>> that's the goal.
>
> Yes, a runtime test.  I don't think there should be an xfail without it
> actually having been tested to fail (and then such an xfail should come
> with a comment referencing the bug filed in Bugzilla).

Would it be ok with the attached testcase then? (same ChangeLog).
Joseph Myers Sept. 11, 2014, 8:28 p.m. UTC | #3
On Thu, 11 Sep 2014, Marc Glisse wrote:

> On Thu, 11 Sep 2014, Joseph S. Myers wrote:
> 
> > On Thu, 11 Sep 2014, Marc Glisse wrote:
> > 
> > > I don't know what kind of test you have in mind, so I added a runtime
> > > test. I
> > > am just guessing that it probably fails on alpha because of PR 58757, I
> > > can't
> > > test. Computing d+d may be even more likely to trigger potential issues,
> > > if
> > > that's the goal.
> > 
> > Yes, a runtime test.  I don't think there should be an xfail without it
> > actually having been tested to fail (and then such an xfail should come
> > with a comment referencing the bug filed in Bugzilla).
> 
> Would it be ok with the attached testcase then? (same ChangeLog).

Yes, OK with that test.
diff mbox

Patch

Index: gcc/c-family/c-cppbuiltin.c
===================================================================
--- gcc/c-family/c-cppbuiltin.c	(revision 215134)
+++ gcc/c-family/c-cppbuiltin.c	(working copy)
@@ -263,35 +263,28 @@  builtin_define_float_constants (const ch
      representable in the given floating point type, b**(1-p).  */
   sprintf (name, "__%s_EPSILON__", name_prefix);
   if (fmt->pnan < fmt->p)
     /* This is an IBM extended double format, so 1.0 + any double is
        representable precisely.  */
       sprintf (buf, "0x1p%d", fmt->emin - fmt->p);
     else
       sprintf (buf, "0x1p%d", 1 - fmt->p);
   builtin_define_with_hex_fp_value (name, type, decimal_dig, buf, fp_suffix, fp_cast);
 
-  /* For C++ std::numeric_limits<T>::denorm_min.  The minimum denormalized
-     positive floating-point number, b**(emin-p).  Zero for formats that
-     don't support denormals.  */
+  /* For C++ std::numeric_limits<T>::denorm_min and C11 *_TRUE_MIN.
+     The minimum denormalized positive floating-point number, b**(emin-p).
+     The minimum normalized positive floating-point number for formats
+     that don't support denormals.  */
   sprintf (name, "__%s_DENORM_MIN__", name_prefix);
-  if (fmt->has_denorm)
-    {
-      sprintf (buf, "0x1p%d", fmt->emin - fmt->p);
-      builtin_define_with_hex_fp_value (name, type, decimal_dig,
-					buf, fp_suffix, fp_cast);
-    }
-  else
-    {
-      sprintf (buf, "0.0%s", fp_suffix);
-      builtin_define_with_value (name, buf, 0);
-    }
+  sprintf (buf, "0x1p%d", fmt->emin - (fmt->has_denorm ? fmt->p : 1));
+  builtin_define_with_hex_fp_value (name, type, decimal_dig,
+				    buf, fp_suffix, fp_cast);
 
   sprintf (name, "__%s_HAS_DENORM__", name_prefix);
   builtin_define_with_value (name, fmt->has_denorm ? "1" : "0", 0);
 
   /* For C++ std::numeric_limits<T>::has_infinity.  */
   sprintf (name, "__%s_HAS_INFINITY__", name_prefix);
   builtin_define_with_int_value (name,
 				 MODE_HAS_INFINITIES (TYPE_MODE (type)));
   /* For C++ std::numeric_limits<T>::has_quiet_NaN.  We do not have a
      predicate to distinguish a target that has both quiet and
Index: gcc/ginclude/float.h
===================================================================
--- gcc/ginclude/float.h	(revision 215134)
+++ gcc/ginclude/float.h	(working copy)
@@ -171,35 +171,23 @@  see the files COPYING3 and COPYING.RUNTI
 #undef DBL_HAS_SUBNORM
 #undef LDBL_HAS_SUBNORM
 #define FLT_HAS_SUBNORM		__FLT_HAS_DENORM__
 #define DBL_HAS_SUBNORM		__DBL_HAS_DENORM__
 #define LDBL_HAS_SUBNORM	__LDBL_HAS_DENORM__
 
 /* Minimum positive values, including subnormals.  */
 #undef FLT_TRUE_MIN
 #undef DBL_TRUE_MIN
 #undef LDBL_TRUE_MIN
-#if __FLT_HAS_DENORM__
 #define FLT_TRUE_MIN	__FLT_DENORM_MIN__
-#else
-#define FLT_TRUE_MIN	__FLT_MIN__
-#endif
-#if __DBL_HAS_DENORM__
 #define DBL_TRUE_MIN	__DBL_DENORM_MIN__
-#else
-#define DBL_TRUE_MIN	__DBL_MIN__
-#endif
-#if __LDBL_HAS_DENORM__
 #define LDBL_TRUE_MIN	__LDBL_DENORM_MIN__
-#else
-#define LDBL_TRUE_MIN	__LDBL_MIN__
-#endif
 
 #endif /* C11 */
 
 #ifdef __STDC_WANT_DEC_FP__
 /* Draft Technical Report 24732, extension for decimal floating-point
    arithmetic: Characteristic of decimal floating types <float.h>.  */
 
 /* Number of base-FLT_RADIX digits in the significand, p.  */
 #undef DEC32_MANT_DIG
 #undef DEC64_MANT_DIG
Index: gcc/testsuite/gcc.dg/c11-true_min-1.c
===================================================================
--- gcc/testsuite/gcc.dg/c11-true_min-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/c11-true_min-1.c	(working copy)
@@ -0,0 +1,17 @@ 
+/* { dg-do run { xfail alpha*-*-* } } */
+/* { dg-options "-std=c11" } */
+
+/* Test that the smallest positive value is not 0. This needs to be true
+   even when denormals are not supported, so we do not pass any flag
+   like -mieee.  */
+
+#include <float.h>
+
+int main(){
+  volatile float f = FLT_TRUE_MIN;
+  volatile double d = DBL_TRUE_MIN;
+  volatile long double l = LDBL_TRUE_MIN;
+  if (f == 0 || d == 0 || l == 0)
+    __builtin_abort ();
+  return 0;
+}