Bug 106831 - [13 Regression] mpfr-4.1.0 started failing 2 tests: tget_set_d64 and tget_set_d128
Summary: [13 Regression] mpfr-4.1.0 started failing 2 tests: tget_set_d64 and tget_set...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 13.0
: P3 normal
Target Milestone: 13.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-09-05 07:18 UTC by Sergei Trofimovich
Modified: 2022-09-18 07:04 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-09-05 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2022-09-05 07:18:31 UTC
The test failures came up on this week's gcc snapshot updates:

- works: gcc-13.0.0.20220828 -O2
- fails: gcc-13.0.0.20220904 -O2
- works: gcc-13.0.0.20220904 -O0

Against https://ftp.gnu.org/gnu/mpfr/mpfr-4.1.0.tar.xz the following seems to be enough:

    $ ./configure CC=gcc-13 && make && make check

Did not extract details yet to double-check if it's really a compiler error (and not an mpfr deficiency). But -O0 / -O2 difference might be a hint.

Two test failures reported:

FAIL: tget_set_d64
==================

Error in check_misc for +0.
  mpfr_get_decimal64() returned: |1011000110100000000000000000000000000000000000000000000000000000|
  mpfr_set_decimal64() set x to: -0 approx.
    = -0
FAIL tget_set_d64 (exit status: 1)

FAIL: tget_set_d128
===================

Error in check_misc for +0.
  mpfr_get_decimal128() returned: 0.0
  mpfr_set_decimal128() set x to: -0 approx.
    = -0
FAIL tget_set_d128 (exit status: 1)

$ gcc -v
Using built-in specs.
COLLECT_GCC=/<<NIX>>/gcc-13.0.0/bin/gcc
COLLECT_LTO_WRAPPER=/<<NIX>>/gcc-13.0.0/libexec/gcc/x86_64-unknown-linux-gnu/13.0.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with:
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 13.0.0 20220904 (experimental) (GCC)
Comment 1 Aldy Hernandez 2022-09-05 08:02:15 UTC
Should be fixed with:

commit 8293a9632c46c8f3f9d531c09194aa8738944927
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Sun Sep 4 08:00:02 2022 +0200

    Do not clobber signbit when unioning a NAN.
    
    When unioning a known NAN and something else, we're dropping the
    properties of the NAN, particularly the sign.  This fixes the
    oversight.
    
    With this patch, we should be keeping the sign bit up to date, even in
    the presence of NANs.

and this:

commit dae8b9e2bbb6017bf90d68c7b720c500125c8295
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Sat Sep 3 15:41:06 2022 +0200

    [PR/middle-end 106819] NANs can never be a singleton
    
    Possible NANs can never be a singleton, so they will never be
    propagated.  This was the intent, and then the signed zero code crept
    in, and was mistakenly checked before the NAN.
Comment 2 Aldy Hernandez 2022-09-05 08:04:17 UTC
And yes, I've started testing mpfr now for my frange patches.
Comment 3 Sergei Trofimovich 2022-09-05 12:05:35 UTC
Hm, mpfr-4.1.0 still fails these 2 tests for me from current gcc-master.

Comparing -O2 build from last week and this week snapshot code generation changed seemingly only for `mpfr_get_decimal64()` at src/.libs/get_d64.o.

I'll re-check current gcc-master, extract self-contained example from mpfr-4.1.0 and reopen the bug if I succeed.
Comment 4 Aldy Hernandez 2022-09-05 12:57:36 UTC
reproduced on x86-64.  Will take a look.  Sorry for closing it prematurely.
Comment 5 Jakub Jelinek 2022-09-05 13:05:43 UTC
BTW, I admit I don't know much about decimal{32,64,128}, but
https://en.wikipedia.org/wiki/Decimal32_floating-point_format
says:
Because the significand is not normalized (there is no implicit leading "1"), most values with less than 7 significant digits have multiple possible representations; 1 × 10^2=0.1 × 10^3=0.01 × 10^4, etc. Zero has 192 possible representations (384 when both signed zeros are included).
So, I think singleton_p should just always return false for DECIMAL_FLOAT_TYPE_P (at least for now).
Comment 6 Aldy Hernandez 2022-09-05 13:35:59 UTC
(In reply to Jakub Jelinek from comment #5)
> BTW, I admit I don't know much about decimal{32,64,128}, but
> https://en.wikipedia.org/wiki/Decimal32_floating-point_format
> says:
> Because the significand is not normalized (there is no implicit leading
> "1"), most values with less than 7 significant digits have multiple possible
> representations; 1 × 10^2=0.1 × 10^3=0.01 × 10^4, etc. Zero has 192 possible
> representations (384 when both signed zeros are included).
> So, I think singleton_p should just always return false for
> DECIMAL_FLOAT_TYPE_P (at least for now).

Interestingly, frange_drop_*inf() bails on DECIMAL_FLOAT_MODE_P because build_real() will ultimately ICE when trying to make a tree out of the max/min representable number for a type:

  /* dconst{0,1,2,m1,half} are used in various places in
     the middle-end and optimizers, allow them here
     even for decimal floating point types as an exception
     by converting them to decimal.  */
  if (DECIMAL_FLOAT_MODE_P (TYPE_MODE (type))
      && (d.cl == rvc_normal || d.cl == rvc_zero)
      && !d.decimal)
...
...

I know even less about decimal floats.  Jakub, should we disable them altogether from the frange infrastructure, or is that too big of a hammer?  I'm just afraid we'll keep running into limitations when we start implementing floating operations in range-op-float.cc.  Or worse, have to special case them all over.

bool frange::supports_p (const_tree type) 
{ 
  return SCALAR_FLOAT_TYPE_P (type) && !DECIMAL_FLOAT_MODE_P (type);
}

??
Comment 7 Jakub Jelinek 2022-09-05 13:48:16 UTC
I guess disabling them at least for now could be fine.
If somebody involved with dfp wants to extend it for dfp, it can be done incrementally.

BTW, thinking further about the singletons, I wonder if MODE_COMPOSITE_P
don't have similar problems.  For the case where the values are exactly equal
to their MSB (i.e. the __ibm128 value is equal to that value cast to double and back), then the low part can be either +0.0 or -0.0.  Ditto for +Inf and -Inf.
So there can be quite a few non-singleton values.
libgcc/config/rs6000/ibm-ldouble-format describes it.
Comment 8 Aldy Hernandez 2022-09-05 13:54:19 UTC
(In reply to Jakub Jelinek from comment #7)
> I guess disabling them at least for now could be fine.
> If somebody involved with dfp wants to extend it for dfp, it can be done
> incrementally.
> 
> BTW, thinking further about the singletons, I wonder if MODE_COMPOSITE_P
> don't have similar problems.  For the case where the values are exactly equal
> to their MSB (i.e. the __ibm128 value is equal to that value cast to double
> and back), then the low part can be either +0.0 or -0.0.  Ditto for +Inf and
> -Inf.
> So there can be quite a few non-singleton values.
> libgcc/config/rs6000/ibm-ldouble-format describes it.

Eeech, could you come up with a patch for that one?  Consider it preapproved :-/.  I'm afraid to mess it up.

In the meantime I think I'll disable decimal floats for frange.
Comment 9 Jakub Jelinek 2022-09-05 14:56:55 UTC
(In reply to Aldy Hernandez from comment #8)
> (In reply to Jakub Jelinek from comment #7)
> > I guess disabling them at least for now could be fine.
> > If somebody involved with dfp wants to extend it for dfp, it can be done
> > incrementally.
> > 
> > BTW, thinking further about the singletons, I wonder if MODE_COMPOSITE_P
> > don't have similar problems.  For the case where the values are exactly equal
> > to their MSB (i.e. the __ibm128 value is equal to that value cast to double
> > and back), then the low part can be either +0.0 or -0.0.  Ditto for +Inf and
> > -Inf.
> > So there can be quite a few non-singleton values.
> > libgcc/config/rs6000/ibm-ldouble-format describes it.
> 
> Eeech, could you come up with a patch for that one?  Consider it preapproved
> :-/.  I'm afraid to mess it up.

I think it would be:
--- gcc/value-range.cc.jj	2022-09-05 16:50:51.443419082 +0200
+++ gcc/value-range.cc	2022-09-05 16:52:04.880434594 +0200
@@ -639,6 +639,19 @@ frange::singleton_p (tree *result) const
       if (HONOR_NANS (m_type) && !get_nan ().no_p ())
 	return false;
 
+      if (MODE_COMPOSITE_P (TYPE_MODE (m_type)))
+	{
+	  // In IBM extended format, if value is +-Inf or
+	  // is exactly representable in double, the other
+	  // double could be +0.0 or -0.0.
+	  if (real_isinf (&m_min))
+	    return false;
+	  REAL_VALUE_TYPE v;
+	  real_convert (&v, DFmode, &m_min);
+	  if (real_identical (&v, &m_min))
+	    return false;
+	}
+
       // Return the appropriate zero if known.
       if (HONOR_SIGNED_ZEROS (m_type) && zero_p ())
 	{
It changes when
long double foo (long double x)
{
  if (x != 25.0L)
    return 16.0L;
  return x + 17.0L;
}
is simplified from evrp to dom2, but perhaps it is ok doing it in dom2 because
the result of the addition would at runtime go through libgcc which would most likely use 0.0 as the other half.
But perhaps it could matter in other cases.
Comment 10 GCC Commits 2022-09-05 15:57:41 UTC
The master branch has been updated by Aldy Hernandez <aldyh@gcc.gnu.org>:

https://gcc.gnu.org/g:b4d8a56a4c62ba8bca55469ae2b841fb4e1334a4

commit r13-2445-gb4d8a56a4c62ba8bca55469ae2b841fb4e1334a4
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Mon Sep 5 15:41:39 2022 +0200

    Disable decimal floating point in frange.
    
    As Jakub mentioned in the PR, because many numbers have multiple
    possible representations, we can't reliably return true for singleton_p.
    For that matter, we may not be capable of modeling them just yet.
    Disabling them until someone with DFP knowledge can opine or extend
    frange.
    
            PR middle-end/106831
    
    gcc/ChangeLog:
    
            * value-range.h (frange::supports_p): Disable decimal floats.
            * range-op-float.cc (frange_drop_inf): Remove DECIMAL_FLOAT_MODE_P
            check.
            (frange_drop_ninf): Same.
Comment 11 Aldy Hernandez 2022-09-05 15:59:21 UTC
Fixed in trunk.  Verified by doing a make check-mpfr with in-tree libmpfr before and after patch.  Thanks Jakub.
Comment 12 jsm-csl@polyomino.org.uk 2022-09-05 19:32:29 UTC
The difference with __ibm128 is that in that case there is no semantic 
significance to whether the low part is +0 or -0, or what the low part is 
at all when the high part is a NaN.  At the C level, such __ibm128 
representations should be considered different representations of the same 
value, not different values.  Whereas different DFP quantum exponents for 
the same real number correspond to different values that compare equal.  
(Noncanonical DFP encodings might be more closely analogous to the 
__ibm128 variants, except that most operations aren't supposed to return a 
noncanonical encoding even if inputs have such an encoding.)
Comment 13 GCC Commits 2022-09-18 07:04:06 UTC
The master branch has been updated by Aldy Hernandez <aldyh@gcc.gnu.org>:

https://gcc.gnu.org/g:5dba8b2a91376d0518eb21c69a0700f099aa9cc4

commit r13-2714-g5dba8b2a91376d0518eb21c69a0700f099aa9cc4
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Sep 17 08:50:22 2022 +0200

    [PR106831] Avoid propagating long doubles that may have multiple representations.
    
    Long doubles are tricky when it comes to considering singletons
    because small numbers and +-INF can have multiple representations for
    the same number.  So we need to be very careful not to treat those as
    singletons, lest they be incorrectly propagated by VRP.  This is
    similar to the -0.0 and +0.0 duality.
    
    In long doubles +INF can be represented with +INF in the MSB and
    either -0.0 or +0.0 in the LSB.  Similarly for numbers that are exactly
    representable in DF.  For example, 1.0 can be represented as either
    (1.0, +0.0) or (1.0, -0.0).
    
    This patch avoids treating these numbers as singletons.
    
    Note that NANs in long double format have a LSB of don't care, but
    this is irrelevant for singleton_p, because NANs are never considered
    singletons.  Also, internally in the frange we store NANs as a pair of
    boolean flags indicating whether they are +NAN or -NAN, so we don't need
    any special treatment here for comparing range equality etc.  We never
    see anything but the boolean flags.
    
            PR middle-end/106831
    
    gcc/ChangeLog:
    
            * value-range.cc (frange::singleton_p): Avoid propagating long
            doubles that may have multiple representations.