[RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Kewen.Lin]

Hi,

This a different attempt from Mike's approach[1][2] to fix the
issue in PR107299.  With option -mabi=ieeelongdouble specified,
type long double (and __float128) and _Float128 have the same
mode TFmode, but they have different type precisions, it causes
the assertion to fail in function fold_using_range::fold_stmt.
IMHO, it's not sensible that two types have the same mode but
different precisions.  By tracing where we make type _Float128
have different precision from the precision of its mode, I found
it's from a work around for rs6000 KFmode.  Being curious why
we need this work around, I disabled it and tested it with some
combinations as below, but all were bootstrapped and no
regression failures were observed.

  - BE Power8 with --with-long-double-format=ibm
  - LE Power9 with --with-long-double-format=ibm
  - LE Power10 with --with-long-double-format=ibm
  - x86_64-redhat-linux
  - aarch64-linux-gnu

For LE Power10 with --with-long-double-format=ieee, since it
suffered the bootstrapping issue, I compared the regression
testing results with the one from Mike's approach.  The
comparison result showed this approach can have several more
test cases pass and no cases regressed, they are:

1) libstdc++:

  FAIL->PASS: std/format/arguments/args.cc (test for excess errors)
  FAIL->PASS: std/format/error.cc (test for excess errors)
  FAIL->PASS: std/format/formatter/requirements.cc (test for excess errors)
  FAIL->PASS: std/format/functions/format.cc (test for excess errors)
  FAIL->PASS: std/format/functions/format_to_n.cc (test for excess errors)
  FAIL->PASS: std/format/functions/size.cc (test for excess errors)
  FAIL->PASS: std/format/functions/vformat_to.cc (test for excess errors)
  FAIL->PASS: std/format/parse_ctx.cc (test for excess errors)
  FAIL->PASS: std/format/string.cc (test for excess errors)
  FAIL->PASS: std/format/string_neg.cc (test for excess errors)

  Caused by the same reason: one static assertion fail in header
  file format (_Type is __ieee128):

    static_assert(__format::__formattable_with<_Type, _Context>);

2) gfortran:

  NA->PASS: gfortran.dg/c-interop/typecodes-array-float128.f90
  NA->PASS: gfortran.dg/c-interop/typecodes-scalar-float128.f90
  NA->PASS: gfortran.dg/PR100914.f90

  Due to that the effective target `fortran_real_c_float128`
  checking fails, fail to compile below source with error msg:
  "Error: Kind -4 not supported for type REAL".

    use iso_c_binding
    real(kind=c_float128) :: x
    x = cos (x)
    end

3) g++:

  FAIL->PASS: g++.dg/cpp23/ext-floating1.C  -std=gnu++23

  Due to the static assertion failing:

    static_assert (is_same<decltype (0.0q), __float128>::value);

* compatible with long double

This approach keeps type long double compatible with _Float128
(at -mabi=ieeelongdouble) as before, so for the simple case
like:

  _Float128 foo (long double t) { return t; }

there is no conversion.  See the difference at optimized
dumping:

  with the contrastive approach:

    _Float128 foo (long double a)
    {
      _Float128 _2;

      <bb 2> [local count: 1073741824]:
      _2 = (_Float128) a_1(D);
      return _2;
  }

  with this:

  _Float128 foo (long double a)
  {
      <bb 2> [local count: 1073741824]:
      return a_1(D);
  }

IMHO, it's still good to keep _Float128 and __float128
compatible with long double, to get rid of those useless
type conversions.

Besides, this approach still takes TFmode attribute type
as type long double, while the contrastive approach takes
TFmode attribute type as type _Float128, whose corresponding
mode isn't TFmode, the inconsistence seems not good.

As above, I wonder if we can consider this approach which
makes type _Float128 have the same precision as MODE_PRECISION
of its mode, it keeps the previous implementation to make type
long double compatible with _Float128.  Since the REAL_MODE_FORMAT
of the mode still holds the information, even if some place which
isn't covered in the above testing need the actual precision, we
can still retrieve the actual precision with that.

Any thoughts?

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604834.html
[2] v3 https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608513.html

BR,
Kewen
-----
	PR target/107299

gcc/ChangeLog:

	* tree.cc (build_common_tree_nodes): Remove workaround for rs6000
	KFmode.
---
 gcc/tree.cc | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/gcc/tree.cc b/gcc/tree.cc
index 254b2373dcf..cbae14d095e 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
       if (!targetm.floatn_mode (n, extended).exists (&mode))
 	continue;
       int precision = GET_MODE_PRECISION (mode);
-      /* Work around the rs6000 KFmode having precision 113 not
-	 128.  */
-      const struct real_format *fmt = REAL_MODE_FORMAT (mode);
-      gcc_assert (fmt->b == 2 && fmt->emin + fmt->emax == 3);
-      int min_precision = fmt->p + ceil_log2 (fmt->emax - fmt->emin);
-      if (!extended)
-	gcc_assert (min_precision == n);
-      if (precision < min_precision)
-	precision = min_precision;
       FLOATN_NX_TYPE_NODE (i) = make_node (REAL_TYPE);
       TYPE_PRECISION (FLOATN_NX_TYPE_NODE (i)) = precision;
       layout_type (FLOATN_NX_TYPE_NODE (i));
--
2.27.0

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Segher Boessenkool]

Hi!

On Wed, Dec 21, 2022 at 05:02:17PM +0800, Kewen.Lin wrote:
> This a different attempt from Mike's approach[1][2] to fix the
> issue in PR107299.

Ke Wen, Mike: so iiuc with this patch applied all three of Mike's
patches are unnecessary?

> With option -mabi=ieeelongdouble specified,
> type long double (and __float128) and _Float128 have the same
> mode TFmode, but they have different type precisions, it causes
> the assertion to fail in function fold_using_range::fold_stmt.
> IMHO, it's not sensible that two types have the same mode but
> different precisions.

Yes, absolutely.  It is a hack, and always was a hack, just one that
worked remarkably well.  But the problems it worked around have not
been fixed yet (and the workarounds are not perfect either).

> By tracing where we make type _Float128
> have different precision from the precision of its mode, I found
> it's from a work around for rs6000 KFmode.  Being curious why
> we need this work around, I disabled it and tested it with some
> combinations as below, but all were bootstrapped and no
> regression failures were observed.
> 
>   - BE Power8 with --with-long-double-format=ibm
>   - LE Power9 with --with-long-double-format=ibm
>   - LE Power10 with --with-long-double-format=ibm
>   - x86_64-redhat-linux
>   - aarch64-linux-gnu
> 
> For LE Power10 with --with-long-double-format=ieee, since it
> suffered the bootstrapping issue, I compared the regression
> testing results with the one from Mike's approach.  The
> comparison result showed this approach can have several more
> test cases pass and no cases regressed, they are:
> 
> 1) libstdc++:
> 
>   FAIL->PASS: std/format/arguments/args.cc (test for excess errors)
>   FAIL->PASS: std/format/error.cc (test for excess errors)
>   FAIL->PASS: std/format/formatter/requirements.cc (test for excess errors)
>   FAIL->PASS: std/format/functions/format.cc (test for excess errors)
>   FAIL->PASS: std/format/functions/format_to_n.cc (test for excess errors)
>   FAIL->PASS: std/format/functions/size.cc (test for excess errors)
>   FAIL->PASS: std/format/functions/vformat_to.cc (test for excess errors)
>   FAIL->PASS: std/format/parse_ctx.cc (test for excess errors)
>   FAIL->PASS: std/format/string.cc (test for excess errors)
>   FAIL->PASS: std/format/string_neg.cc (test for excess errors)
> 
>   Caused by the same reason: one static assertion fail in header
>   file format (_Type is __ieee128):
> 
>     static_assert(__format::__formattable_with<_Type, _Context>);
> 
> 2) gfortran:
> 
>   NA->PASS: gfortran.dg/c-interop/typecodes-array-float128.f90
>   NA->PASS: gfortran.dg/c-interop/typecodes-scalar-float128.f90
>   NA->PASS: gfortran.dg/PR100914.f90
> 
>   Due to that the effective target `fortran_real_c_float128`
>   checking fails, fail to compile below source with error msg:
>   "Error: Kind -4 not supported for type REAL".
> 
>     use iso_c_binding
>     real(kind=c_float128) :: x
>     x = cos (x)
>     end
> 
> 3) g++:
> 
>   FAIL->PASS: g++.dg/cpp23/ext-floating1.C  -std=gnu++23
> 
>   Due to the static assertion failing:
> 
>     static_assert (is_same<decltype (0.0q), __float128>::value);

Does it fix the new testcases in Mike's series as well?

> * compatible with long double
> 
> This approach keeps type long double compatible with _Float128
> (at -mabi=ieeelongdouble) as before, so for the simple case
> like:
> 
>   _Float128 foo (long double t) { return t; }
> 
> there is no conversion.  See the difference at optimized
> dumping:
> 
>   with the contrastive approach:
> 
>     _Float128 foo (long double a)
>     {
>       _Float128 _2;
> 
>       <bb 2> [local count: 1073741824]:
>       _2 = (_Float128) a_1(D);
>       return _2;
>   }
> 
>   with this:
> 
>   _Float128 foo (long double a)
>   {
>       <bb 2> [local count: 1073741824]:
>       return a_1(D);
>   }
> 
> IMHO, it's still good to keep _Float128 and __float128
> compatible with long double, to get rid of those useless
> type conversions.
> 
> Besides, this approach still takes TFmode attribute type
> as type long double, while the contrastive approach takes
> TFmode attribute type as type _Float128, whose corresponding
> mode isn't TFmode, the inconsistence seems not good.
> 
> As above, I wonder if we can consider this approach which
> makes type _Float128 have the same precision as MODE_PRECISION
> of its mode, it keeps the previous implementation to make type
> long double compatible with _Float128.  Since the REAL_MODE_FORMAT
> of the mode still holds the information, even if some place which
> isn't covered in the above testing need the actual precision, we
> can still retrieve the actual precision with that.

"Precision" does not have a useful meaning for all floating point
formats.  It does not have one for double-double for example.  The way
precision is defined in IEEE FP means double-double has 2048 bits of
precision (or is it 2047), not useful.  Taking the precision of the
format instead to be the minimum over all values in that format gives
double-double the same precision as IEEE DP, not useful in any practical
way either :-/

> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
>        if (!targetm.floatn_mode (n, extended).exists (&mode))
>  	continue;
>        int precision = GET_MODE_PRECISION (mode);
> -      /* Work around the rs6000 KFmode having precision 113 not
> -	 128.  */

It has precision 126 now fwiw.

Joseph: what do you think about this patch?  Is the workaround it
removes still useful in any way, do we need to do that some other way if
we remove this?


Segher

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Joseph Myers]

On Wed, 21 Dec 2022, Segher Boessenkool wrote:

> > --- a/gcc/tree.cc
> > +++ b/gcc/tree.cc
> > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> >  	continue;
> >        int precision = GET_MODE_PRECISION (mode);
> > -      /* Work around the rs6000 KFmode having precision 113 not
> > -	 128.  */
> 
> It has precision 126 now fwiw.
> 
> Joseph: what do you think about this patch?  Is the workaround it
> removes still useful in any way, do we need to do that some other way if
> we remove this?

I think it's best for the TYPE_PRECISION, for any type with the binary128 
format, to be 128 (not 126).

It's necessary that _Float128, _Float64x and long double all have the same 
TYPE_PRECISION when they have the same (binary128) format, or at least 
that TYPE_PRECISION for _Float128 >= that for long double >= that for 
_Float64x, so that the rules in c_common_type apply properly.

How the TYPE_PRECISION compares to that of __ibm128, or of long double 
when that's double-double, is less important.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Jakub Jelinek]

On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> 
> > > --- a/gcc/tree.cc
> > > +++ b/gcc/tree.cc
> > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> > >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> > >  	continue;
> > >        int precision = GET_MODE_PRECISION (mode);
> > > -      /* Work around the rs6000 KFmode having precision 113 not
> > > -	 128.  */
> > 
> > It has precision 126 now fwiw.
> > 
> > Joseph: what do you think about this patch?  Is the workaround it
> > removes still useful in any way, do we need to do that some other way if
> > we remove this?
> 
> I think it's best for the TYPE_PRECISION, for any type with the binary128 
> format, to be 128 (not 126).

Agreed.

> It's necessary that _Float128, _Float64x and long double all have the same 
> TYPE_PRECISION when they have the same (binary128) format, or at least 
> that TYPE_PRECISION for _Float128 >= that for long double >= that for 
> _Float64x, so that the rules in c_common_type apply properly.
> 
> How the TYPE_PRECISION compares to that of __ibm128, or of long double 
> when that's double-double, is less important.

I guess it can affect the common type for {long double
(when binary128),_Float128,_Float64x,__float128,__ieee128} vs. {long double (when
ibm128),__ibm128}, especially in C (for C++ only when non-standard types are
involved (__float128, __ieee128, __ibm128).
But I think unless we error (e.g. in C++ when we see unordered floating
point types), prefering binary128 is better, it certainly has much bigger
exponent range over __ibm128 and most of the time also the precision
(__ibm128 wastes some bits on the other exponent).

	Jakub

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Kewen.Lin]

Hi Joseph,

on 2022/12/22 05:40, Joseph Myers wrote:
> On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> 
>>> --- a/gcc/tree.cc
>>> +++ b/gcc/tree.cc
>>> @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
>>>        if (!targetm.floatn_mode (n, extended).exists (&mode))
>>>  	continue;
>>>        int precision = GET_MODE_PRECISION (mode);
>>> -      /* Work around the rs6000 KFmode having precision 113 not
>>> -	 128.  */
>>
>> It has precision 126 now fwiw.
>>
>> Joseph: what do you think about this patch?  Is the workaround it
>> removes still useful in any way, do we need to do that some other way if
>> we remove this?
> 
> I think it's best for the TYPE_PRECISION, for any type with the binary128 
> format, to be 128 (not 126).

I agree that it's more reasonable to use 128 for it (all of them) eventually,
but what I thought is that if we can get rid of this workaround first to make
the bootstrapping survive.  Commit r9-1302-g6a8886e45f7eb6 makes TFmode/
KFmode/IFmode have different precisions with some reasons, Jakub mentioned
commit r13-3292, I think later we can refer to it and have a try to unique
those modes to have the same precisions (probably next stage1), I guess Mike
may have more insightful comments here.

> 
> It's necessary that _Float128, _Float64x and long double all have the same 
> TYPE_PRECISION when they have the same (binary128) format, or at least 
> that TYPE_PRECISION for _Float128 >= that for long double >= that for 
> _Float64x, so that the rules in c_common_type apply properly.
> 

a. _Float128, _Float64x and long double all have the same
   TYPE_PRECISION when they have the same (binary128) format.

b. TYPE_PRECISION for _Float128 >= that for long double
   >= that for _Float64x.

**Without this patch**, 

1) -mabi=ieeelongdouble (these three are ieee128)

  _Float128 => 128 ("type => its TYPE_PRECISION")
  _Float64x => 128
  long double => 127

// Neither a and b holds.

2) -mabi=ibmlongdouble (long double is ibm128)

  _Float128 => 128
  _Float64x => 128
  long double => 127

// a N/A, b doesn't hold.

**With this patch**, 

1) -mabi=ieeelongdouble

  _Float128 => 127
  _Float64x => 127
  long double => 127

// both a and b hold.
  
2) -mabi=ibmlongdouble
  
  _Float128 => 126
  _Float64x => 126
  long double => 127

// a N/A, b doesn't hold.

IMHO, this patch improves the status quo slightly.

BR,
Kewen

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Segher Boessenkool]

Hi!

On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> > > --- a/gcc/tree.cc
> > > +++ b/gcc/tree.cc
> > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> > >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> > >  	continue;
> > >        int precision = GET_MODE_PRECISION (mode);
> > > -      /* Work around the rs6000 KFmode having precision 113 not
> > > -	 128.  */
> > 
> > It has precision 126 now fwiw.
> > 
> > Joseph: what do you think about this patch?  Is the workaround it
> > removes still useful in any way, do we need to do that some other way if
> > we remove this?

You didn't address these questions.  We don't see negative effects from
removing this workaround, but it isn't clear (to me) what problems were
there that caused you to do this workaround.  Do you remember maybe?  Or
can we just delete it and try to forget such worries :-)

> I think it's best for the TYPE_PRECISION, for any type with the binary128 
> format, to be 128 (not 126).

Well, but why?  Of course it looks nicer, and it is a gross workaround
to have different precisions for the different 128-bit FP modes, more so
if two modes are really the same, but in none of the ways floating point
precision is defined would it be 128 for any 128-bit mode.

> It's necessary that _Float128, _Float64x and long double all have the same 
> TYPE_PRECISION when they have the same (binary128) format,

Yes, agreed.  Or even if it would be not necessary it is the only sane
thing to do.


Segher

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Joseph Myers]

On Thu, 22 Dec 2022, Segher Boessenkool wrote:

> Hi!
> 
> On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> > On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> > > > --- a/gcc/tree.cc
> > > > +++ b/gcc/tree.cc
> > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> > > >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> > > >  	continue;
> > > >        int precision = GET_MODE_PRECISION (mode);
> > > > -      /* Work around the rs6000 KFmode having precision 113 not
> > > > -	 128.  */
> > > 
> > > It has precision 126 now fwiw.
> > > 
> > > Joseph: what do you think about this patch?  Is the workaround it
> > > removes still useful in any way, do we need to do that some other way if
> > > we remove this?
> 
> You didn't address these questions.  We don't see negative effects from
> removing this workaround, but it isn't clear (to me) what problems were
> there that caused you to do this workaround.  Do you remember maybe?  Or
> can we just delete it and try to forget such worries :-)

The purpose was to ensure that _Float128's TYPE_PRECISION was at least as 
large as that of long double, in the case where they both have binary128 
format.  I think at that time, in GCC 7, it was possible for _Float128 to 
be KFmode and long double to be TFmode, with those being different modes 
with the same format.

In my view, it would be best not to have different modes with the same 
format - not simply ensure types with the same format have the same mode, 
but avoid multiple modes with the same format existing in the compiler at 
all.  That is, TFmode should be the same mode as one of KFmode and IFmode 
(one name should be defined as a macro for the other name, or something 
similar).  If you don't have different modes with the same format, many of 
the problems go away.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Segher Boessenkool]

On Thu, Dec 22, 2022 at 07:48:28PM +0000, Joseph Myers wrote:
> On Thu, 22 Dec 2022, Segher Boessenkool wrote:
> > On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> > > On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> > > > Joseph: what do you think about this patch?  Is the workaround it
> > > > removes still useful in any way, do we need to do that some other way if
> > > > we remove this?
> > 
> > You didn't address these questions.  We don't see negative effects from
> > removing this workaround, but it isn't clear (to me) what problems were
> > there that caused you to do this workaround.  Do you remember maybe?  Or
> > can we just delete it and try to forget such worries :-)
> 
> The purpose was to ensure that _Float128's TYPE_PRECISION was at least as 
> large as that of long double, in the case where they both have binary128 
> format.  I think at that time, in GCC 7, it was possible for _Float128 to 
> be KFmode and long double to be TFmode, with those being different modes 
> with the same format.

They still are separate modes :-(  It always is possible to create
KFmode entities (via mode((KF)) if nothing else) and those should behave
exactly the same as TFmode if TFmode is IEEE QP (just like KFmode always
is).

> In my view, it would be best not to have different modes with the same 
> format - not simply ensure types with the same format have the same mode, 
> but avoid multiple modes with the same format existing in the compiler at 
> all.  That is, TFmode should be the same mode as one of KFmode and IFmode 
> (one name should be defined as a macro for the other name, or something 
> similar).

Right, TFmode should be just a different *name* for either IFmode or
KFmode (and both of those modes always exist if either does).

> If you don't have different modes with the same format, many of 
> the problems go away.

I used to have patches for this.  A few problems remained, but this
was very long ago, who knows where we stand now.  I'll recreate those
patches, let's see where that gets us.

Thanks for the help,


Segher

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Michael Meissner]

On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> 
> > > --- a/gcc/tree.cc
> > > +++ b/gcc/tree.cc
> > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> > >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> > >  	continue;
> > >        int precision = GET_MODE_PRECISION (mode);
> > > -      /* Work around the rs6000 KFmode having precision 113 not
> > > -	 128.  */
> > 
> > It has precision 126 now fwiw.
> > 
> > Joseph: what do you think about this patch?  Is the workaround it
> > removes still useful in any way, do we need to do that some other way if
> > we remove this?
> 
> I think it's best for the TYPE_PRECISION, for any type with the binary128 
> format, to be 128 (not 126).
> 
> It's necessary that _Float128, _Float64x and long double all have the same 
> TYPE_PRECISION when they have the same (binary128) format, or at least 
> that TYPE_PRECISION for _Float128 >= that for long double >= that for 
> _Float64x, so that the rules in c_common_type apply properly.
> 
> How the TYPE_PRECISION compares to that of __ibm128, or of long double 
> when that's double-double, is less important.

When I did the original implementation years ago, there were various implicit
assumptions that for any one precision, there must be only one floating point
type.

I tend to agree that logically the precision should be 128, but until we go
through and fix all of these assumptions, it may be problematical.  This shows
up in the whole infrastructure of looking for a FP type with larger precision
than a given precision.  There just isn't an ordering that works and preserves
all values.

I'm coming to think that we may want 2 types of FP, one is a standard FP type
where you can convert to a larger type, and the other for various FP types
where there is no default widening conversion.

And logically there is the issue with 16-bit floats, giving we have different
versions of 16-bit float.

And if an implementation ever wanted to support both BID and DFP decimal types
at the same time, they would have similar issues.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Michael Meissner]

On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> 
> > > --- a/gcc/tree.cc
> > > +++ b/gcc/tree.cc
> > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> > >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> > >  	continue;
> > >        int precision = GET_MODE_PRECISION (mode);
> > > -      /* Work around the rs6000 KFmode having precision 113 not
> > > -	 128.  */
> > 
> > It has precision 126 now fwiw.
> > 
> > Joseph: what do you think about this patch?  Is the workaround it
> > removes still useful in any way, do we need to do that some other way if
> > we remove this?
> 
> I think it's best for the TYPE_PRECISION, for any type with the binary128 
> format, to be 128 (not 126).
> 
> It's necessary that _Float128, _Float64x and long double all have the same 
> TYPE_PRECISION when they have the same (binary128) format, or at least 
> that TYPE_PRECISION for _Float128 >= that for long double >= that for 
> _Float64x, so that the rules in c_common_type apply properly.
> 
> How the TYPE_PRECISION compares to that of __ibm128, or of long double 
> when that's double-double, is less important.

I spent a few days on working on this.  I have patches to make the 3 128-bit
types to all have TYPE_PRECISION of 128.  To do this, I added a new mode macro
(FRACTIONAL_FLOAT_MODE_NO_WIDEN) that takes the same arguments as
FRACTIONAL_FLOAT_MODE.

This will create a floating point mode that is a normal floating point mode,
but the GET_MODE_WIDER and GET_MODE_2XWIDER macros will never return it.  By
declaring both IFmode and KFmode to not be widened to, but noral TFmode is, it
eliminates the problems where an IBM expression got converted to IEEE, which
can mostly (but not always) contain the value.  In addition, on power8, it
means it won't call the KF mode emulator functions, just the IF functions.

We need to have one 128-bit mode (TFmode) that is not declared as NO_WARN, or
long double won't be created, since float_mode_for_size (128) will not be able
to find the correct type.

I did have to patch convert_mode_scalar so that it would not abort if it was
doing a conversion between two floating point types with the same precision.

I tested this with the first patch from the previous set of patches (that
rewrites complex multiply/divide built-in setup).  I think that patch is useful
as a stand alone patch.

I also used Kewen Lin's patch from December 27th in build_common_tree_nodes to
do the test.  I haven't tested if this particular patch fixes this problem, or
it fixes something else.

Finally, I used the third patch in my series of patches that straightens out
128<->128 FP conversions.  That patch needed to be tweaked slightly, as one of
the conversations became FLOAT_EXTEND instead of FLOAT_TRUNCATE.

We don't have a RTL operation that says convert from one floating point type to
another where both types are the same size.  Whether FLOAT_EXTEND is used or
FLOAT_TRUNCATE, used to depend on whether the TYPE_PRECISION was greater or
lesser.  Now that they are the same, it arbitrarily picks FLOAT_EXTEND.

While I still think the 2nd patch is important, it isn't needed with the above
patches.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Michael Meissner]

On Fri, Jan 06, 2023 at 07:41:07PM -0500, Michael Meissner wrote:
> On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote:
> > On Wed, 21 Dec 2022, Segher Boessenkool wrote:
> > 
> > > > --- a/gcc/tree.cc
> > > > +++ b/gcc/tree.cc
> > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
> > > >        if (!targetm.floatn_mode (n, extended).exists (&mode))
> > > >  	continue;
> > > >        int precision = GET_MODE_PRECISION (mode);
> > > > -      /* Work around the rs6000 KFmode having precision 113 not
> > > > -	 128.  */
> > > 
> > > It has precision 126 now fwiw.
> > > 
> > > Joseph: what do you think about this patch?  Is the workaround it
> > > removes still useful in any way, do we need to do that some other way if
> > > we remove this?
> > 
> > I think it's best for the TYPE_PRECISION, for any type with the binary128 
> > format, to be 128 (not 126).
> > 
> > It's necessary that _Float128, _Float64x and long double all have the same 
> > TYPE_PRECISION when they have the same (binary128) format, or at least 
> > that TYPE_PRECISION for _Float128 >= that for long double >= that for 
> > _Float64x, so that the rules in c_common_type apply properly.
> > 
> > How the TYPE_PRECISION compares to that of __ibm128, or of long double 
> > when that's double-double, is less important.
> 
> I spent a few days on working on this.  I have patches to make the 3 128-bit
> types to all have TYPE_PRECISION of 128.  To do this, I added a new mode macro
> (FRACTIONAL_FLOAT_MODE_NO_WIDEN) that takes the same arguments as
> FRACTIONAL_FLOAT_MODE.

...

I had the patches to change the precision to 128, and I just ran them.  C and
C++ do not seem to be bothered by changing the precision to 128 (once I got it
to build, etc.).  But Fortran on the other hand does actually use the precision
to differentiate between IBM extended double and IEEE 128-bit.  In particular,
the following 3 tests fail when long double is IBM extended double:

	gfortran.dg/PR100914.f90
	gfortran.dg/c-interop/typecodes-array-float128.f90
	gfortran.dg/c-interop/typecodes-scalar-float128.f90

I tried adding code to use the old precisions for Fortran, but not for C/C++,
but it didn't seem to work.

So while it might be possible to use a single 128 for the precision, it needs
more work and attention, particularly on the Fortran side.

I'm not sure it is worth it to try and change things.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Jakub Jelinek]

On Mon, Jan 09, 2023 at 10:21:52PM -0500, Michael Meissner wrote:
> I had the patches to change the precision to 128, and I just ran them.  C and
> C++ do not seem to be bothered by changing the precision to 128 (once I got it
> to build, etc.).  But Fortran on the other hand does actually use the precision
> to differentiate between IBM extended double and IEEE 128-bit.  In particular,
> the following 3 tests fail when long double is IBM extended double:
> 
> 	gfortran.dg/PR100914.f90
> 	gfortran.dg/c-interop/typecodes-array-float128.f90
> 	gfortran.dg/c-interop/typecodes-scalar-float128.f90
> 
> I tried adding code to use the old precisions for Fortran, but not for C/C++,
> but it didn't seem to work.
> 
> So while it might be possible to use a single 128 for the precision, it needs
> more work and attention, particularly on the Fortran side.

Can't be more than a few lines changed in the fortran FE.
Yes, the FE needs to know if it is IBM extended double or IEEE 128-bit so
that it can decide on the mangling - where to use the artificial kind 17 and
where to use 16.  But as long as it can figure that out, it doesn't need to
rely on a particular precision.

	Jakub

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Michael Meissner]

On Tue, Jan 10, 2023 at 07:23:23PM +0100, Jakub Jelinek wrote:
> On Mon, Jan 09, 2023 at 10:21:52PM -0500, Michael Meissner wrote:
> > I had the patches to change the precision to 128, and I just ran them.  C and
> > C++ do not seem to be bothered by changing the precision to 128 (once I got it
> > to build, etc.).  But Fortran on the other hand does actually use the precision
> > to differentiate between IBM extended double and IEEE 128-bit.  In particular,
> > the following 3 tests fail when long double is IBM extended double:
> > 
> > 	gfortran.dg/PR100914.f90
> > 	gfortran.dg/c-interop/typecodes-array-float128.f90
> > 	gfortran.dg/c-interop/typecodes-scalar-float128.f90
> > 
> > I tried adding code to use the old precisions for Fortran, but not for C/C++,
> > but it didn't seem to work.
> > 
> > So while it might be possible to use a single 128 for the precision, it needs
> > more work and attention, particularly on the Fortran side.
> 
> Can't be more than a few lines changed in the fortran FE.
> Yes, the FE needs to know if it is IBM extended double or IEEE 128-bit so
> that it can decide on the mangling - where to use the artificial kind 17 and
> where to use 16.  But as long as it can figure that out, it doesn't need to
> rely on a particular precision.

I agree that in theory it should be simple to fix.  Unfortunately the patches
that I was working on cause some other failures that I need to investigate.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]

[Kewen.Lin]

Hi Segher,

on 2022/12/22 05:24, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Dec 21, 2022 at 05:02:17PM +0800, Kewen.Lin wrote:
>> This a different attempt from Mike's approach[1][2] to fix the
>> issue in PR107299.
> 
> Ke Wen, Mike: so iiuc with this patch applied all three of Mike's
> patches are unnecessary?

I think the 1st patch is still needed, it's to fix a latent issue
as the associated test cases {mul,div}ic3-1.c show.

> Does it fix the new testcases in Mike's series as well?

Yeah, it doesn't suffer the issue exposed by float128-hw4.c, so
it doesn't need that adjustment on float128-hw4.c.  It can also
make newly added float128-hw{12,13}.c pass.

>> As above, I wonder if we can consider this approach which
>> makes type _Float128 have the same precision as MODE_PRECISION
>> of its mode, it keeps the previous implementation to make type
>> long double compatible with _Float128.  Since the REAL_MODE_FORMAT
>> of the mode still holds the information, even if some place which
>> isn't covered in the above testing need the actual precision, we
>> can still retrieve the actual precision with that.
> 
> "Precision" does not have a useful meaning for all floating point
> formats.  It does not have one for double-double for example.  The way
> precision is defined in IEEE FP means double-double has 2048 bits of
> precision (or is it 2047), not useful.  Taking the precision of the
> format instead to be the minimum over all values in that format gives
> double-double the same precision as IEEE DP, not useful in any practical
> way either :-/

OK, I think that's why we don't see any regressions with this work
around removal,   :)

> 
>> --- a/gcc/tree.cc
>> +++ b/gcc/tree.cc
>> @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char)
>>        if (!targetm.floatn_mode (n, extended).exists (&mode))
>>  	continue;
>>        int precision = GET_MODE_PRECISION (mode);
>> -      /* Work around the rs6000 KFmode having precision 113 not
>> -	 128.  */
> 
> It has precision 126 now fwiw.

Yes, with -mabi=ibmlongdouble, it uses KFmode so 126, while with
-mabi=ieeelongdouble, it uses TFmode so 127.

BR,
Kewen