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

[Segher Boessenkool <segher@kernel.crashing.org>]

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