Hi Gaius,
on 2024/6/5 22:22, Gaius Mulley wrote:
> "Kewen.Lin" <li...@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
>
