Hi!
On Sat, Mar 05, 2022 at 09:21:51AM +0100, Jakub Jelinek wrote:
> As mentioned in the PR, right now on powerpc* __SIZEOF_{FLOAT,IBM}128__
> macros are predefined unconditionally, because {ieee,ibm}128_float_type_node
> is always non-NULL, doesn't reflect whether __ieee128 or __ibm128 are
> actually supported or not.
>
> The following patch:
> 1) makes those {ieee,ibm}128_float_type_node trees NULL if we don't
> really support them instead of equal to long_double_type_node
> 2) adjusts the builtins code to use
> ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> for the 2 builtins, so that we don't ICE during builtins creation
> if ibm128_float_type_node is NULL (using error_mark_node instead of
> long_double_type_node sounds more risky to me)
I feel the opposite way: (potentially) using the wrong thing is just a
ticking time bomb, never "safer".
> 3) in rs6000_type_string will not match NULL as __ibm128, and adds
> a __ieee128 case
> 4) removes the clearly unused ptr_{ieee,ibm}128_float_type_node trees;
> if something needs them in the future, they can be easily added back,
> but wasting GTY just in case...
> 5) actually syncs __SIZEOF_FLOAT128__ with the __float128 macro being
> defined in addition to __ieee128 being usable
>
> Now, in the PR Segher said he doesn't like 5), but I think it is better
> to match the reality and get this PR fixed and if we want to rethink
> how __float128 is defined (whether it is a macro, or perhaps another
> builtin type like __ieee128 which could be easily done by
> lang_hooks.types.register_builtin_type (ieee128_float_type_node,
> "__ieee128");
> lang_hooks.types.register_builtin_type (ieee128_float_type_node,
> "__float128");
> perhaps under some conditions, rethink if the -mfloat128 option exists
> and what it does etc., it can be done incrementally and the rs6000-c.cc
> hunks in the patch can be easily reverted (appart from the formatting
> fix).
There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)
Sorry I did not pick up on that earlier.
The basic types, that always exist if they are supported at all, are
__ibm128 and __ieee128. Long double picks one of those, or some other
type; and __float128 is a source of confusion (it tries to make things
more similar to x86, but things are *not* similar anyway :-( )
Any code that cares about the actual mode used, or that supports more
than one type, should use the explicit types. Code that just wants what
"long double" stands for can just use that, of course.
> Bootstrapped/regtested on powerpc{64le,64}-linux, on powerpc64-linux
> with -m32/-m64 testing, ok for trunk?
Tested with which -mcpu=? You need at least power7 to get __ieee128
support, but it should be tested *without* that as well. (The actual
default for powerpc64-linux is power4 (so not even 970), and for
powerpc-linux it is 603 or such.)
> * config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> __SIZEOF_FLOAT128__ here and only if __float128 macro is defined.
(if and only if?)
> * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> 85 instead of 86.
This should be auto-generated, or just not exist at all
("sizeof type_map / sizeof type_map[0]" in the one place it is used,
instead -- "one place" is after removing it from the definition of the
array of course, where it is unneeded :-) )
> static
> const char *rs6000_type_string (tree type_node)
> {
> + if (type_node == NULL_TREE)
> + return "unknown";
Please say this is NULL? If you want to indicate it is problematic, you
can say "***NULL***" or such?
> + { "if", "ibm128_float_type_node "
> + "? ibm128_float_type_node "
> + ": long_double" },
I don't think that is correct. If there is no __ibm128, there is no
IFmode either (they are two sides of the same coin). Using DFmode
instead (which is the only thing that "long double" could be if not
IFmode) will just ICE later, if this is used at all. So best case this
will confuse the reader.
Segher
