Re: [PATCH 06/52] m2: Replace uses of {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Gaius,

on 2024/6/5 22:22, Gaius Mulley wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
> 
>> Hi Joseph and Gaius,
>>
>> on 2024/6/4 02:02, Joseph Myers wrote:
>>> On Sun, 2 Jun 2024, Kewen Lin wrote:
>>>
>>>> diff --git a/gcc/m2/gm2-gcc/m2type.cc b/gcc/m2/gm2-gcc/m2type.cc
>>>> index 571923c08ef..d52cbdf0b99 100644
>>>> --- a/gcc/m2/gm2-gcc/m2type.cc
>>>> +++ b/gcc/m2/gm2-gcc/m2type.cc
>>>> @@ -1420,7 +1420,7 @@ build_m2_short_real_node (void)
>>>>    /* Define `REAL'.  */
>>>>  
>>>>    c = make_node (REAL_TYPE);
>>>> -  TYPE_PRECISION (c) = FLOAT_TYPE_SIZE;
>>>> +  TYPE_PRECISION (c) = TYPE_PRECISION (float_type_node);
>>>>    layout_type (c);
>>>>    return c;
>>>>  }
>>>> @@ -1433,7 +1433,7 @@ build_m2_real_node (void)
>>>>    /* Define `REAL'.  */
>>>>  
>>>>    c = make_node (REAL_TYPE);
>>>> -  TYPE_PRECISION (c) = DOUBLE_TYPE_SIZE;
>>>> +  TYPE_PRECISION (c) = TYPE_PRECISION (double_type_node);
>>>>    layout_type (c);
>>>>    return c;
>>>>  }
>>>> @@ -1447,7 +1447,7 @@ build_m2_long_real_node (void)
>>>>    if (M2Options_GetIBMLongDouble ())
>>>>      {
>>>>        longreal = make_node (REAL_TYPE);
>>>> -      TYPE_PRECISION (longreal) = LONG_DOUBLE_TYPE_SIZE;
>>>> +      TYPE_PRECISION (longreal) = TYPE_PRECISION (long_double_type_node);
>>>
>>> This looks rather like m2 would still have the same problem the generic 
>>> code previously had: going via precision when that might not uniquely 
>>> determine the desired machine mode.  And so making sure to use the right 
>>> machine mode as done when setting up long_double_type_node etc. would be 
>>> better than keeping this code copying TYPE_PRECISION and hoping to 
>>> determine a machine mode from that.  It certainly looks like this code 
>>> wants to match float, double and long double, rather than possibly getting 
>>> a different mode with possibly the same TYPE_PRECISION.
>>
>> Good point, sorry that I just did a replacement without checking the context.
>> If the above holds (Gaius can confirm or clarify), SET_TYPE_MODE would be
>> also applied here, that is:
> 
> Hi Kewen and Joseph,
> 
> yes the code is attempting to create nodes SHORTREAL, REAL, LONGREAL
> mapping onto the C float, double, long double nodes.
> 
>> diff --git a/gcc/m2/gm2-gcc/m2type.cc b/gcc/m2/gm2-gcc/m2type.cc
>> index d52cbdf0b99..5ff02a18876 100644
>> --- a/gcc/m2/gm2-gcc/m2type.cc
>> +++ b/gcc/m2/gm2-gcc/m2type.cc
>> @@ -1421,6 +1421,7 @@ build_m2_short_real_node (void)
>>
>>    c = make_node (REAL_TYPE);
>>    TYPE_PRECISION (c) = TYPE_PRECISION (float_type_node);
>> +  SET_TYPE_MODE (c, TYPE_MODE (float_type_node));
>>    layout_type (c);
>>    return c;
>>  }
>> @@ -1434,6 +1435,7 @@ build_m2_real_node (void)
>>
>>    c = make_node (REAL_TYPE);
>>    TYPE_PRECISION (c) = TYPE_PRECISION (double_type_node);
>> +  SET_TYPE_MODE (c, TYPE_MODE (double_type_node));
>>    layout_type (c);
>>    return c;
>>  }
>>
>> I'm not sure and curious why the above builds new nodes for short real and
>> real but re-use float128_type_node or long_double_type_node for some cases,
>> some special needs cause the former ones should have separated nodes?
> 
> good point - there is no need to create new nodes.

ah, ok, thanks for clarifying!

> 
>>> (I don't know if the M2Options_GetIBMLongDouble call would be needed at 
>>> all once you use the machine mode for long double in a reliable way, or 
>>> whether this code could be further simplified.)
>>
>> long_double_type_node should already take care of ibmlongdouble, IIUC it
>> would be like:
>>
>> @@ -1443,13 +1445,7 @@ build_m2_long_real_node (void)
>>  {
>>    tree longreal;
>>
>> -  /* Define `LONGREAL'.  */
>> -  if (M2Options_GetIBMLongDouble ())
>> -    {
>> -      longreal = make_node (REAL_TYPE);
>> -      TYPE_PRECISION (longreal) = TYPE_PRECISION (long_double_type_node);
>> -    }
>> -  else if (M2Options_GetIEEELongDouble ())
>> +  if (M2Options_GetIEEELongDouble ())
>>      longreal = float128_type_node;
>>    else
>>      longreal = long_double_type_node;
> 
> yes indeed and the above patch is fine, all bootstrapped and regression
> tested on ppc64le (cfarm120).  I've also bootstrapped and regression tested
> the following patch on ppc64le and aarch64:

Nice!  Looking forward to you pushing this new one (I'm withdrawing the original
patch).

> 
> diff --git a/gcc/m2/gm2-gcc/m2type.cc b/gcc/m2/gm2-gcc/m2type.cc
> index 571923c08ef..ce53130e2a9 100644
> --- a/gcc/m2/gm2-gcc/m2type.cc
> +++ b/gcc/m2/gm2-gcc/m2type.cc
> @@ -1415,41 +1415,26 @@ build_m2_char_node (void)
>  static tree
>  build_m2_short_real_node (void)
>  {
> -  tree c;
> -
> -  /* Define `REAL'.  */
> -
> -  c = make_node (REAL_TYPE);
> -  TYPE_PRECISION (c) = FLOAT_TYPE_SIZE;
> -  layout_type (c);
> -  return c;
> +  /* Define `SHORTREAL'.  */
> +  layout_type (float_type_node);

It looks that float_type_node, double_type_node, float128_type_node and
long_double_type_node have been called with layout_type when they are
being initialized in function build_common_tree_nodes, maybe we can just
assert their TYPE_SIZE.

BR,
Kewen

> +  return float_type_node;
>  }
>  
>  static tree
>  build_m2_real_node (void)
>  {
> -  tree c;
> -
>    /* Define `REAL'.  */
> -
> -  c = make_node (REAL_TYPE);
> -  TYPE_PRECISION (c) = DOUBLE_TYPE_SIZE;
> -  layout_type (c);
> -  return c;
> +  layout_type (double_type_node);
> +  return double_type_node;
>  }
>  
>  static tree
>  build_m2_long_real_node (void)
>  {
>    tree longreal;
> -
> +  
>    /* Define `LONGREAL'.  */
> -  if (M2Options_GetIBMLongDouble ())
> -    {
> -      longreal = make_node (REAL_TYPE);
> -      TYPE_PRECISION (longreal) = LONG_DOUBLE_TYPE_SIZE;
> -    }
> -  else if (M2Options_GetIEEELongDouble ())
> +  if (M2Options_GetIEEELongDouble ())
>      longreal = float128_type_node;
>    else
>      longreal = long_double_type_node;
> 
> 
> regards,
> Gaius
>