diff mbox series

range: Workaround different type precision issue between _Float128 and long double [PR112788]

Message ID b40da958-d5ee-817b-3e0c-30cb4bc3a091@linux.ibm.com
State Unresolved
Headers
Series range: Workaround different type precision issue between _Float128 and long double [PR112788] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Kewen.Lin Dec. 4, 2023, 9:49 a.m. UTC 
  Hi,

As PR112788 shows, on rs6000 with -mabi=ieeelongdouble type _Float128
has the different type precision (128) from that (127) of type long
double, but actually they has the same underlying mode, so they have
the same precision as the mode indicates the same real type format
ieee_quad_format.

It's not sensible to have such two types which have the same mode but
different type precisions, some fix attempt was posted at [1].
As the discussion there, there are some historical reasons and
practical issues.  Considering we passed stage 1 and it also affected
the build as reported, this patch is trying to temporarily workaround
it.  I thought to introduce a hookpod but that seems a bit overkill,
assuming scalar float type with the same mode should have the same
precision looks sensible.

Bootstrapped and regtested on powerpc64-linux-gnu P7/P8/P9 and
powerpc64le-linux-gnu P9/P10.

Is it ok for trunk?

[1] https://inbox.sourceware.org/gcc-patches/718677e7-614d-7977-312d-05a75e1fd5b4@linux.ibm.com/

BR,
Kewen
----
	PR tree-optimization/112788

gcc/ChangeLog:

	* value-range.h (range_compatible_p): Workaround same type mode but
	different type precision issue for rs6000 scalar float types
	_Float128 and long double.
---
 gcc/value-range.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--
2.42.0

Comments

Kewen.Lin Dec. 12, 2023, 6:07 a.m. UTC | #1 
Hi,

Gentle ping this:

https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639140.html

BR,
Kewen

on 2023/12/4 17:49, Kewen.Lin wrote:
> Hi,
> 
> As PR112788 shows, on rs6000 with -mabi=ieeelongdouble type _Float128
> has the different type precision (128) from that (127) of type long
> double, but actually they has the same underlying mode, so they have
> the same precision as the mode indicates the same real type format
> ieee_quad_format.
> 
> It's not sensible to have such two types which have the same mode but
> different type precisions, some fix attempt was posted at [1].
> As the discussion there, there are some historical reasons and
> practical issues.  Considering we passed stage 1 and it also affected
> the build as reported, this patch is trying to temporarily workaround
> it.  I thought to introduce a hookpod but that seems a bit overkill,
> assuming scalar float type with the same mode should have the same
> precision looks sensible.
> 
> Bootstrapped and regtested on powerpc64-linux-gnu P7/P8/P9 and
> powerpc64le-linux-gnu P9/P10.
> 
> Is it ok for trunk?
> 
> [1] https://inbox.sourceware.org/gcc-patches/718677e7-614d-7977-312d-05a75e1fd5b4@linux.ibm.com/
> 
> BR,
> Kewen
> ----
> 	PR tree-optimization/112788
> 
> gcc/ChangeLog:
> 
> 	* value-range.h (range_compatible_p): Workaround same type mode but
> 	different type precision issue for rs6000 scalar float types
> 	_Float128 and long double.
> ---
>  gcc/value-range.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/value-range.h b/gcc/value-range.h
> index 33f204a7171..d0a84754a10 100644
> --- a/gcc/value-range.h
> +++ b/gcc/value-range.h
> @@ -1558,7 +1558,13 @@ range_compatible_p (tree type1, tree type2)
>    // types_compatible_p requires conversion in both directions to be useless.
>    // GIMPLE only requires a cast one way in order to be compatible.
>    // Ranges really only need the sign and precision to be the same.
> -  return (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
> -	  && TYPE_SIGN (type1) == TYPE_SIGN (type2));
> +  return TYPE_SIGN (type1) == TYPE_SIGN (type2)
> +	 && (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
> +	     // FIXME: As PR112788 shows, for now on rs6000 _Float128 has
> +	     // type precision 128 while long double has type precision 127
> +	     // but both have the same mode so their precision is actually
> +	     // the same, workaround it temporarily.
> +	     || (SCALAR_FLOAT_TYPE_P (type1)
> +		 && TYPE_MODE (type1) == TYPE_MODE (type2)));
>  }
>  #endif // GCC_VALUE_RANGE_H
> --
> 2.42.0
>
Andrew MacLeod Dec. 12, 2023, 2:33 p.m. UTC | #2 
I leave this for the release managers, but I am not opposed to it for 
this release... It would be nice to remove it for the next release

Andrew



On 12/12/23 01:07, Kewen.Lin wrote:
> Hi,
>
> Gentle ping this:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639140.html
>
> BR,
> Kewen
>
> on 2023/12/4 17:49, Kewen.Lin wrote:
>> Hi,
>>
>> As PR112788 shows, on rs6000 with -mabi=ieeelongdouble type _Float128
>> has the different type precision (128) from that (127) of type long
>> double, but actually they has the same underlying mode, so they have
>> the same precision as the mode indicates the same real type format
>> ieee_quad_format.
>>
>> It's not sensible to have such two types which have the same mode but
>> different type precisions, some fix attempt was posted at [1].
>> As the discussion there, there are some historical reasons and
>> practical issues.  Considering we passed stage 1 and it also affected
>> the build as reported, this patch is trying to temporarily workaround
>> it.  I thought to introduce a hookpod but that seems a bit overkill,
>> assuming scalar float type with the same mode should have the same
>> precision looks sensible.
>>
>> Bootstrapped and regtested on powerpc64-linux-gnu P7/P8/P9 and
>> powerpc64le-linux-gnu P9/P10.
>>
>> Is it ok for trunk?
>>
>> [1] https://inbox.sourceware.org/gcc-patches/718677e7-614d-7977-312d-05a75e1fd5b4@linux.ibm.com/
>>
>> BR,
>> Kewen
>> ----
>> 	PR tree-optimization/112788
>>
>> gcc/ChangeLog:
>>
>> 	* value-range.h (range_compatible_p): Workaround same type mode but
>> 	different type precision issue for rs6000 scalar float types
>> 	_Float128 and long double.
>> ---
>>   gcc/value-range.h | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/gcc/value-range.h b/gcc/value-range.h
>> index 33f204a7171..d0a84754a10 100644
>> --- a/gcc/value-range.h
>> +++ b/gcc/value-range.h
>> @@ -1558,7 +1558,13 @@ range_compatible_p (tree type1, tree type2)
>>     // types_compatible_p requires conversion in both directions to be useless.
>>     // GIMPLE only requires a cast one way in order to be compatible.
>>     // Ranges really only need the sign and precision to be the same.
>> -  return (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
>> -	  && TYPE_SIGN (type1) == TYPE_SIGN (type2));
>> +  return TYPE_SIGN (type1) == TYPE_SIGN (type2)
>> +	 && (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
>> +	     // FIXME: As PR112788 shows, for now on rs6000 _Float128 has
>> +	     // type precision 128 while long double has type precision 127
>> +	     // but both have the same mode so their precision is actually
>> +	     // the same, workaround it temporarily.
>> +	     || (SCALAR_FLOAT_TYPE_P (type1)
>> +		 && TYPE_MODE (type1) == TYPE_MODE (type2)));
>>   }
>>   #endif // GCC_VALUE_RANGE_H
>> --
>> 2.42.0
>>
Jakub Jelinek Dec. 12, 2023, 2:42 p.m. UTC | #3 
On Tue, Dec 12, 2023 at 09:33:38AM -0500, Andrew MacLeod wrote:
> I leave this for the release managers, but I am not opposed to it for this
> release... It would be nice to remove it for the next release

I can live with it for GCC 14, so ok, but it is very ugly.

We should fix it in a better way for GCC 15+.
I think we shouldn't lie, both on the mode precisions and on type
precisions.  The middle-end already contains some hacks to make it
work to some extent on 2 different modes with same precision (for BFmode vs.
HFmode), on the FE side if we need a target hook the C/C++ FE will use
to choose type ranks and/or the type for binary operations, so be it.
It would be also great if rs6000 backend had just 2 modes for 128-bit
floats, one for IBM double double, one for IEEE quad, not 3 as it has now,
perhaps with TFmode being a macro that conditionally expands to one or the
other.  Or do some tweaks in target hooks to keep backwards compatibility
with mode attribute and similar.

	Jakub
Kewen.Lin Dec. 13, 2023, 2:42 a.m. UTC | #4 
Hi Jakub & Andrew,

on 2023/12/12 22:42, Jakub Jelinek wrote:
> On Tue, Dec 12, 2023 at 09:33:38AM -0500, Andrew MacLeod wrote:
>> I leave this for the release managers, but I am not opposed to it for this
>> release... It would be nice to remove it for the next release
> 
> I can live with it for GCC 14, so ok, but it is very ugly.

Thanks, pushed as r14-6478-gfda8e2f8292a90.

And yes, I strongly agree that we should get rid of this in next release.

> 
> We should fix it in a better way for GCC 15+.
> I think we shouldn't lie, both on the mode precisions and on type
> precisions.  The middle-end already contains some hacks to make it
> work to some extent on 2 different modes with same precision (for BFmode vs.
> HFmode), on the FE side if we need a target hook the C/C++ FE will use
> to choose type ranks and/or the type for binary operations, so be it.
> It would be also great if rs6000 backend had just 2 modes for 128-bit
> floats, one for IBM double double, one for IEEE quad, not 3 as it has now,
> perhaps with TFmode being a macro that conditionally expands to one or the
> other.  Or do some tweaks in target hooks to keep backwards compatibility
> with mode attribute and similar.

Thanks for all the insightful suggestions, I just filed PR112993 for
further tracking and self-assigned it.

BR,
Kewen
diff mbox series

Patch

diff --git a/gcc/value-range.h b/gcc/value-range.h
index 33f204a7171..d0a84754a10 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -1558,7 +1558,13 @@  range_compatible_p (tree type1, tree type2)
   // types_compatible_p requires conversion in both directions to be useless.
   // GIMPLE only requires a cast one way in order to be compatible.
   // Ranges really only need the sign and precision to be the same.
-  return (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
-	  && TYPE_SIGN (type1) == TYPE_SIGN (type2));
+  return TYPE_SIGN (type1) == TYPE_SIGN (type2)
+	 && (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
+	     // FIXME: As PR112788 shows, for now on rs6000 _Float128 has
+	     // type precision 128 while long double has type precision 127
+	     // but both have the same mode so their precision is actually
+	     // the same, workaround it temporarily.
+	     || (SCALAR_FLOAT_TYPE_P (type1)
+		 && TYPE_MODE (type1) == TYPE_MODE (type2)));
 }
 #endif // GCC_VALUE_RANGE_H